From 032efb1c28fb0f538cedaa9615fb50efe1e918e6 Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Fri, 18 Sep 2026 12:03:05 +0800 Subject: [PATCH 1/4] Load chat preferences asynchronously with bounded retryable initialization Centralize account-scoped preference state in lifecycle-owned storage. Restore mode, model and history through guarded Realm publication; keep preference-dependent controls disabled until ready and show loading, failure and retry states. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../eclipse/core/chat/UserPreference.java | 5 +- .../chat/services/ChatBaseServiceTests.java | 152 +++- .../ui/chat/services/ModelServiceTests.java | 34 +- .../chat/services/PreferenceStorageTest.java | 761 ++++++++++++++++++ .../services/UserPreferenceServiceTest.java | 346 +++++--- .../copilot/eclipse/ui/chat/ActionBar.java | 43 +- .../copilot/eclipse/ui/chat/ChatView.java | 8 + .../eclipse/ui/chat/HandoffContainer.java | 16 + .../copilot/eclipse/ui/chat/Messages.java | 4 + .../eclipse/ui/chat/PreferenceStatus.java | 64 ++ .../eclipse/ui/chat/messages.properties | 5 + .../ui/chat/services/ChatBaseService.java | 97 +-- .../ui/chat/services/ChatServiceManager.java | 16 +- .../ui/chat/services/ModelService.java | 242 ++++-- .../ui/chat/services/PreferenceStorage.java | 489 +++++++++++ .../chat/services/UserPreferenceService.java | 149 ++-- 16 files changed, 2028 insertions(+), 403 deletions(-) create mode 100644 com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java create mode 100644 com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java create mode 100644 com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/UserPreference.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/UserPreference.java index 0d5cded98..ccf643a0f 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/UserPreference.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/UserPreference.java @@ -12,9 +12,8 @@ /** - * Preferences per GitHub user. All the getters and setters are synchronized due to that the ChatBaseService holds a - * shared (single) reference to the user preference. synchronized modifies makes sure the update to the instance are - * thread safe. + * Preferences per GitHub user, shared by the chat preference storage. Scalar access is synchronized and model option + * maps use immutable snapshots so readers never observe partially updated maps. */ public class UserPreference { diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseServiceTests.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseServiceTests.java index 751291b2c..3adc0f118 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseServiceTests.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseServiceTests.java @@ -3,19 +3,39 @@ package com.microsoft.copilot.eclipse.ui.chat.services; -import static org.mockito.Mockito.verify; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; -import org.junit.jupiter.api.BeforeEach; +import java.util.List; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; + +import org.eclipse.swt.SWT; +import org.eclipse.swt.widgets.Display; +import org.eclipse.swt.widgets.Shell; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; import org.mockito.Mock; -import org.mockito.MockitoAnnotations; +import org.mockito.junit.jupiter.MockitoExtension; import com.microsoft.copilot.eclipse.core.AuthStatusManager; -import com.microsoft.copilot.eclipse.core.CopilotCore; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; +import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotModel; +import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotScope; +import com.microsoft.copilot.eclipse.core.lsp.protocol.byok.ByokListModelResponse; +import com.microsoft.copilot.eclipse.core.lsp.protocol.quota.CheckQuotaResult; +import com.microsoft.copilot.eclipse.ui.swt.DropdownButton; +@ExtendWith(MockitoExtension.class) class ChatBaseServiceTests { @Mock @@ -24,40 +44,110 @@ class ChatBaseServiceTests { @Mock private AuthStatusManager mockAuthStatusManager; - @Mock - private CopilotCore mockCopilotCore; - - private TestChatBaseService chatBaseService; - - @BeforeEach - void setUp() { - MockitoAnnotations.openMocks(this); - chatBaseService = new TestChatBaseService(mockLsConnection, mockAuthStatusManager); - } + @TempDir + private Path directory; @Test void persistUserPreference_WhenNotSignedIn_ShouldSkipLspInteraction() { - // Arrange when(mockAuthStatusManager.isSignedIn()).thenReturn(false); - - // Act - chatBaseService.persistUserPreference(); - - // Assert - verify(mockAuthStatusManager).isSignedIn(); - verifyNoInteractions(mockLsConnection); // LSP connection should not be used + PreferenceStorage storage = new PreferenceStorage(mockLsConnection, mockAuthStatusManager); + try { + storage.initialize(); + storage.persist(); + assertEquals(PreferenceStorage.State.UNAVAILABLE, storage.getState()); + verifyNoInteractions(mockLsConnection); + } finally { + storage.dispose(); + } } - /** - * Test implementation of ChatBaseService for testing purposes - */ - private static class TestChatBaseService extends ChatBaseService { - public TestChatBaseService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager) { - super(lsConnection, authStatusManager); - } + @Test + void testFirstInitialization_PendingAuthenticatedPersistence_DoesNotBlockSwt() throws Exception { + when(mockAuthStatusManager.isSignedIn()).thenReturn(true); + when(mockAuthStatusManager.getUserName()).thenReturn("pending-user"); + when(mockAuthStatusManager.getQuotaStatus()).thenReturn(CheckQuotaResult.empty()); + CompletableFuture persistence = new CompletableFuture<>(); + when(mockLsConnection.persistence()).thenReturn(persistence); + CopilotModel model = new CopilotModel(); + model.setId("saved-model"); + model.setModelName("Saved model"); + model.setModelFamily("family"); + model.setScopes(List.of(CopilotScope.CHAT_PANEL, CopilotScope.AGENT_PANEL)); + when(mockLsConnection.listModels()).thenReturn(CompletableFuture.completedFuture(new CopilotModel[] {model})); + Path file = directory.resolve("pending-user").resolve("pref.json"); + Files.createDirectories(file.getParent()); + String saved = "{\"chatModeName\":\"Ask\",\"chatModel\":\"" + model.getModelKey() + "\"}"; + Files.writeString(file, saved); + ByokListModelResponse byok = new ByokListModelResponse(); + byok.setModels(List.of()); + when(mockLsConnection.listByokModels(org.mockito.ArgumentMatchers.any())) + .thenReturn(CompletableFuture.completedFuture(byok)); - public void persistUserPreference() { - super.persistUserPreference(); + AtomicReference storage = new AtomicReference<>(); + AtomicReference preferences = new AtomicReference<>(); + AtomicReference models = new AtomicReference<>(); + AtomicReference shell = new AtomicReference<>(); + AtomicReference modeControl = new AtomicReference<>(); + AtomicReference modelControl = new AtomicReference<>(); + CompletableFuture uiAction = new CompletableFuture<>(); + Display.getDefault().asyncExec(() -> { + try { + storage.set(new PreferenceStorage(mockLsConnection, mockAuthStatusManager)); + models.set(new ModelService(mockLsConnection, mockAuthStatusManager, storage.get())); + preferences.set(new UserPreferenceService(mockLsConnection, mockAuthStatusManager, storage.get())); + shell.set(new Shell(Display.getDefault())); + DropdownButton modePicker = new DropdownButton(shell.get(), SWT.NONE); + DropdownButton modelPicker = new DropdownButton(shell.get(), SWT.NONE); + modeControl.set(modePicker); + modelControl.set(modelPicker); + preferences.get().bindChatModePicker(modePicker); + models.get().bindModelPicker(modelPicker); + Display.getDefault().asyncExec(() -> { + uiAction.complete(!modePicker.getEnabled() && !modelPicker.getEnabled() + && storage.get().getState() == PreferenceStorage.State.LOADING); + }); + } catch (Throwable error) { + uiAction.completeExceptionally(error); + } + }); + try { + assertTrue(uiAction.get(5, TimeUnit.SECONDS), "Preference controls must stay disabled while SWT processes work"); + assertFalse(persistence.isDone(), "SWT must process another action before persistence completes"); + Display.getDefault().syncExec(() -> assertNull(models.get().getActiveModel())); + ChatPersistence response = new ChatPersistence(); + response.setPath(directory.toString()); + persistence.complete(response); + AtomicReference restored = new AtomicReference<>(false); + long deadline = System.nanoTime() + TimeUnit.SECONDS.toNanos(5); + do { + Display.getDefault().syncExec(() -> restored.set( + "Ask".equals(preferences.get().getActiveModeNameOrId()) + && models.get().getActiveModel() == model + && modeControl.get().getEnabled() && modelControl.get().getEnabled())); + if (restored.get()) { + break; + } + Thread.sleep(10); + } while (System.nanoTime() < deadline); + assertTrue(restored.get(), "Mode and model must restore after the shared load succeeds"); + assertEquals(saved, Files.readString(file), "Placeholder observables must not save over stored preferences"); + } finally { + // Release a regressed blocking constructor before cleaning up on SWT. + persistence.completeExceptionally(new IllegalStateException("test cleanup")); + Display.getDefault().syncExec(() -> { + if (shell.get() != null) { + shell.get().dispose(); + } + if (preferences.get() != null) { + preferences.get().dispose(); + } + if (models.get() != null) { + models.get().dispose(); + } + if (storage.get() != null) { + storage.get().dispose(); + } + }); } } } \ No newline at end of file diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java index 1626af9a8..2141f01ff 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java @@ -59,13 +59,12 @@ class ModelServiceTests { private Path persistenceDirectory; private ModelService modelService; + private PreferenceStorage preferenceStorage; private FeatureFlags featureFlags; private boolean previewFeaturesEnabled; @BeforeEach void setUp() { - new PreferenceCacheResetter(lsConnection, authStatusManager).reset(); - ChatPersistence persistence = new ChatPersistence(); persistence.setPath(persistenceDirectory.toString()); ByokListModelResponse byokModels = new ByokListModelResponse(); @@ -80,6 +79,7 @@ void setUp() { assertNotNull(featureFlags); previewFeaturesEnabled = featureFlags.isClientPreviewFeatureEnabled(); featureFlags.setClientPreviewFeatureEnabled(false); + preferenceStorage = new PreferenceStorage(lsConnection, authStatusManager); } @AfterEach @@ -88,7 +88,7 @@ void tearDown() { modelService.dispose(); } featureFlags.setClientPreviewFeatureEnabled(previewFeaturesEnabled); - new PreferenceCacheResetter(lsConnection, authStatusManager).reset(); + preferenceStorage.dispose(); } @Test @@ -98,7 +98,7 @@ void testAutoModelAvailableWhenEditorPreviewDisabled() throws InterruptedExcepti when(lsConnection.listModels()) .thenReturn(CompletableFuture.completedFuture(new CopilotModel[] { defaultModel, autoModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> isModelAvailable(defaultModel.getModelKey())); assertTrue(isModelAvailable(autoModel.getModelKey())); @@ -112,7 +112,7 @@ void testAutoModelPolicyChangeRefreshesModelInventory() throws InterruptedExcept CompletableFuture.completedFuture(new CopilotModel[] { defaultModel, autoModel }), CompletableFuture.completedFuture(new CopilotModel[] { defaultModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> isModelAvailable(autoModel.getModelKey())); IEventBroker eventBroker = PlatformUI.getWorkbench().getService(IEventBroker.class); @@ -133,7 +133,7 @@ void testAutoPolicyDisableSelectsServerDefaultAndKeepsAutoPreference() CompletableFuture.completedFuture(new CopilotModel[] { defaultModel, autoModel, otherModel }), CompletableFuture.completedFuture(new CopilotModel[] { otherModel, defaultModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> autoModel.getId().equals(getActiveModelId())); IEventBroker eventBroker = PlatformUI.getWorkbench().getService(IEventBroker.class); @@ -155,7 +155,7 @@ void testAutoPolicyDisableUsesDeterministicFallbackAndKeepsAutoPreference() CompletableFuture.completedFuture(new CopilotModel[] { autoModel, lastModel, firstModel }), CompletableFuture.completedFuture(new CopilotModel[] { lastModel, firstModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> autoModel.getId().equals(getActiveModelId())); IEventBroker eventBroker = PlatformUI.getWorkbench().getService(IEventBroker.class); @@ -176,7 +176,7 @@ void testAutoPolicyReEnableRestoresPersistedAutoPreference() throws IOException, CompletableFuture.completedFuture(new CopilotModel[] { defaultModel }), CompletableFuture.completedFuture(new CopilotModel[] { defaultModel, autoModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> autoModel.getId().equals(getActiveModelId())); IEventBroker eventBroker = PlatformUI.getWorkbench().getService(IEventBroker.class); @@ -197,7 +197,7 @@ void testDefaultKeyCollisionSelectsModelFromCurrentInventory() throws IOExceptio when(lsConnection.listModels()) .thenReturn(CompletableFuture.completedFuture(new CopilotModel[] { defaultModel, inventoryModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> isModelAvailable(defaultModel.getModelKey())); AtomicReference activeModel = new AtomicReference<>(); @@ -217,7 +217,7 @@ void testSetActiveModel_AlreadyActiveModelDoesNotPersistPreference() throws Inte when(lsConnection.listModels()) .thenReturn(CompletableFuture.completedFuture(new CopilotModel[] { defaultModel })); - modelService = new ModelService(lsConnection, authStatusManager); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); waitUntil(() -> defaultModel.getId().equals(getActiveModelId())); AtomicReference activeModel = new AtomicReference<>(); @@ -290,18 +290,6 @@ private String readPersistedModel() throws IOException { } private Path getPreferenceFile() { - return persistenceDirectory.resolve(TEST_USER).resolve(ChatBaseService.PREF_FILE_NAME); - } - - private static final class PreferenceCacheResetter extends ChatBaseService { - - private PreferenceCacheResetter(CopilotLanguageServerConnection lsConnection, - AuthStatusManager authStatusManager) { - super(lsConnection, authStatusManager); - } - - private void reset() { - clearUserPreferenceCache(); - } + return persistenceDirectory.resolve(TEST_USER).resolve("pref.json"); } } diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java new file mode 100644 index 000000000..52a9ab84f --- /dev/null +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java @@ -0,0 +1,761 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.ui.chat.services; + +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.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.io.IOException; +import java.nio.file.AccessDeniedException; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.util.Queue; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicLong; + +import org.eclipse.core.databinding.observable.Realm; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledIf; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.ArgumentCaptor; +import org.mockito.Mockito; + +import com.microsoft.copilot.eclipse.core.AuthStatusManager; +import com.microsoft.copilot.eclipse.core.CopilotAuthStatusListener; +import com.microsoft.copilot.eclipse.core.chat.UserPreference; +import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; +import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult; +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage.State; + +class PreferenceStorageTest { + private final CopilotLanguageServerConnection connection = mock(CopilotLanguageServerConnection.class); + private final AuthStatusManager auth = mock(AuthStatusManager.class); + private final ExecutorService worker = mock(ExecutorService.class); + private final ScheduledExecutorService timer = mock(ScheduledExecutorService.class); + private final TestFileAccess files = mock(TestFileAccess.class); + private final Queue work = new ConcurrentLinkedQueue<>(); + private final Queue deadlines = new ConcurrentLinkedQueue<>(); + private final TestRealm realm = new TestRealm(); + private final AtomicLong clock = new AtomicLong(); + private CompletableFuture response; + private PreferenceStorage storage; + + @BeforeEach + void setUp() { + when(auth.isSignedIn()).thenReturn(true); + when(auth.getUserName()).thenReturn("alice"); + response = new CompletableFuture<>(); + when(connection.persistence()).thenAnswer(invocation -> response); + Mockito.doAnswer(invocation -> { + work.add(invocation.getArgument(0)); + return null; + }).when(worker).execute(any(Runnable.class)); + when(timer.schedule(any(Runnable.class), anyLong(), any(TimeUnit.class))).thenAnswer(invocation -> { + deadlines.add(invocation.getArgument(0)); + return mock(ScheduledFuture.class); + }); + storage = new PreferenceStorage(connection, auth, realm, worker, timer, clock::get, files); + } + + @AfterEach + void tearDown() { + storage.dispose(); + realm.drain(); + } + + @Test + void testInitialize_SavedPreferences_RestoresOnlyInRealm() throws Exception { + when(files.read(any(Path.class))).thenReturn(""" + {"chatModel":"model-a","chatModeName":"Ask","userInputs":["hello"], + "skipGitHubJobConfirmDialog":true,"reasoningEffortByModel":{"model-a":"high"}, + "contextWindowByModel":{"model-a":128000}} + """); + + storage.initialize(); + assertEquals(State.LOADING, storage.getState()); + assertNull(storage.getReadyPreferences()); + drainWork(); + response.complete(persistence()); + drainWork(); + assertEquals(State.LOADING, storage.getState()); + realm.drain(); + + assertEquals(State.READY, storage.getReadiness().getValue()); + UserPreference restored = storage.getReadyPreferences(); + assertEquals("model-a", restored.getChatModel()); + assertEquals("Ask", restored.getChatModeName()); + assertEquals("hello", restored.getUserInputs().get(0)); + assertEquals(true, restored.isSkipGitHubJobConfirmDialog()); + assertEquals("high", restored.getReasoningEffort("model-a")); + assertEquals(128000, restored.getContextWindow("model-a")); + assertSame(restored, storage.getReadyPreferences()); + } + + @Test + void testInitialize_MissingFile_UsesFirstRunDefaultsWithoutWriting() throws Exception { + when(files.read(any(Path.class))).thenThrow(new NoSuchFileException("pref.json")); + load(); + + assertEquals(State.READY, storage.getState()); + assertNotNull(storage.getReadyPreferences()); + assertNull(storage.getReadyPreferences().getChatModel()); + assertTrue(storage.getReadyPreferences().getReasoningEffortSnapshot().isEmpty()); + verify(files, never()).write(any(), any()); + } + + @Test + void testInitialize_UnreadableFile_FailsAndDoesNotOverwrite() throws Exception { + when(files.read(any(Path.class))).thenThrow(new AccessDeniedException("pref.json")); + load(); + + assertFailedWithoutWrites(); + storage.initialize(); + drainWork(); + verify(connection).persistence(); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" ", "null", "[]", "{", "{\"chatModel\":\"x\"} garbage", "{chatModel:'x'}", + "{\"chatModel\":\"x\",}", "/*comment*/{}", "{\"chatModel\":{}}", "{\"contextWindowByModel\":false}"}) + void testInitialize_InvalidJson_FailsAndDoesNotOverwrite(String content) throws Exception { + when(files.read(any(Path.class))).thenReturn(content); + load(); + + assertFailedWithoutWrites(); + } + + @Test + void testInitialize_NullModelMaps_NormalizesSafeDefaults() throws Exception { + when(files.read(any(Path.class))).thenReturn(""" + {"reasoningEffortByModel":null,"contextWindowByModel":null} + """); + load(); + + assertEquals(State.READY, storage.getState()); + UserPreference restored = storage.getReadyPreferences(); + assertNull(restored.getReasoningEffort("model-a")); + assertNull(restored.getContextWindow("model-a")); + restored.setReasoningEffort("model-a", "high"); + assertTrue(restored.setContextWindow("model-a", 128000)); + } + + @Test + @EnabledIf("supportsStrictJson") + void testInitialize_GsonSupportsStrictJson_RejectsUnescapedControlCharacters() throws Exception { + when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"line\nbreak\"}"); + load(); + + assertFailedWithoutWrites(); + } + + private static boolean supportsStrictJson() { + try { + Class.forName("com.google.gson.Strictness"); + return true; + } catch (ClassNotFoundException exception) { + return false; + } + } + + @Test + void testInitialize_PendingRpc_LeavesUiPublicationAndGettersAvailable() { + storage.initialize(); + drainWork(); + realm.drain(); + + assertEquals(State.LOADING, storage.getReadiness().getValue()); + assertNull(storage.getReadyPreferences()); + assertFalse(response.isDone()); + verifyNoInteractions(files); + verify(timer).schedule(any(Runnable.class), eq(15L), eq(TimeUnit.SECONDS)); + } + + @Test + void testInitialize_ConcurrentCallersAndRetry_CoalescesPendingAndReadyLoads() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + storage.initialize(); + storage.initialize(); + storage.retry(); + storage.retry(); + drainWork(); + response.complete(persistence()); + drainWork(); + realm.drain(); + UserPreference restored = storage.getReadyPreferences(); + restored.setChatModel("session-choice"); + storage.initialize(); + storage.retry(); + drainWork(); + + verify(connection).persistence(); + assertSame(restored, storage.getReadyPreferences()); + assertEquals("session-choice", storage.getReadyPreferences().getChatModel()); + } + + @Test + void testInitialize_ParallelCallers_ShareOnePendingRequest() throws Exception { + CompletableFuture[] callers = new CompletableFuture[12]; + for (int i = 0; i < callers.length; i++) { + callers[i] = CompletableFuture.runAsync(storage::initialize); + } + CompletableFuture.allOf(callers).get(5, TimeUnit.SECONDS); + drainWork(); + realm.drain(); + + assertEquals(State.LOADING, storage.getReadiness().getValue()); + assertNull(storage.getReadyPreferences()); + verify(connection).persistence(); + } + + @Test + void testInitialize_RpcFailure_RequiresExplicitRetry() throws Exception { + storage.initialize(); + drainWork(); + response.completeExceptionally(new IOException("offline")); + realm.drain(); + assertFailedWithoutWrites(); + storage.initialize(); + drainWork(); + verify(connection).persistence(); + + response = new CompletableFuture<>(); + when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"recovered\"}"); + storage.retry(); + storage.retry(); + drainWork(); + response.complete(persistence()); + drainWork(); + realm.drain(); + + assertEquals("recovered", storage.getReadyPreferences().getChatModel()); + verify(connection, times(2)).persistence(); + } + + @Test + void testInitialize_NullRpcResponse_Fails() throws Exception { + storage.initialize(); + drainWork(); + response.complete(null); + drainWork(); + realm.drain(); + + assertFailedWithoutWrites(); + verify(files, never()).read(any()); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" ", "relative-path", "invalid\u0000path"}) + void testInitialize_InvalidPersistencePath_Fails(String path) throws Exception { + storage.initialize(); + drainWork(); + ChatPersistence invalid = new ChatPersistence(); + invalid.setPath(path); + response.complete(invalid); + drainWork(); + realm.drain(); + + assertFailedWithoutWrites(); + verify(files, never()).read(any()); + } + + @Test + void testInitialize_RpcInvocationThrows_FailsWithoutReading() throws Exception { + when(connection.persistence()).thenThrow(new IllegalStateException("unavailable")); + storage.initialize(); + drainWork(); + realm.drain(); + + assertFailedWithoutWrites(); + verify(files, never()).read(any()); + } + + @Test + void testInitialize_NullRpcFuture_FailsWithoutReading() throws Exception { + when(connection.persistence()).thenReturn(null); + storage.initialize(); + drainWork(); + realm.drain(); + + assertFailedWithoutWrites(); + verify(files, never()).read(any()); + } + + @Test + void testDeadline_PendingRpc_FailsAndCancelsRequest() throws Exception { + storage.initialize(); + drainWork(); + expire(); + realm.drain(); + + assertFailedWithoutWrites(); + assertTrue(response.isCancelled()); + } + + @Test + void testDeadline_BeforeWorkerDispatch_DoesNotStartRpc() throws Exception { + storage.initialize(); + expire(); + drainWork(); + realm.drain(); + + assertFailedWithoutWrites(); + verify(connection, never()).persistence(); + } + + @Test + void testDeadline_DelayedTimer_StillRejectsPublicationAfterDeadline() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + storage.initialize(); + drainWork(); + response.complete(persistence()); + drainWork(); + clock.set(TimeUnit.SECONDS.toNanos(15)); + realm.drain(); + + assertFailedWithoutWrites(); + } + + @Test + void testDeadline_DuringRead_RejectsLateReadResult() throws Exception { + when(files.read(any(Path.class))).thenAnswer(invocation -> { + expire(); + return "{\"chatModel\":\"too-late\"}"; + }); + load(); + + assertFailedWithoutWrites(); + } + + @Test + void testRetry_LateReadFromTimedOutAttempt_DoesNotReplaceSuccessfulRetry() throws Exception { + when(files.read(any(Path.class))).thenAnswer(invocation -> { + Mockito.doReturn("{\"chatModel\":\"new\"}").when(files).read(any(Path.class)); + expire(); + response = new CompletableFuture<>(); + storage.retry(); + drainWork(); + response.complete(persistence()); + drainWork(); + realm.drain(); + return "{\"chatModel\":\"old\"}"; + }); + load(); + + assertEquals(State.READY, storage.getState()); + assertEquals("new", storage.getReadyPreferences().getChatModel()); + verify(connection, times(2)).persistence(); + } + + @Test + void testInitialize_DuringFileRead_GettersRemainAvailableToOtherThreads() throws Exception { + when(files.read(any(Path.class))).thenAnswer(invocation -> { + assertEquals(State.LOADING, CompletableFuture.supplyAsync(storage::getState).get(5, TimeUnit.SECONDS)); + assertNull(CompletableFuture.supplyAsync(storage::getReadyPreferences).get(5, TimeUnit.SECONDS)); + return "{}"; + }); + load(); + + assertEquals(State.READY, storage.getState()); + } + + @Test + void testRetry_LateRpcFromTimedOutAttempt_CannotReplaceNewPreferences() throws Exception { + response = new UncancellableFuture(); + CompletableFuture old = response; + storage.initialize(); + drainWork(); + expire(); + realm.drain(); + + response = new CompletableFuture<>(); + when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"new\"}"); + storage.retry(); + drainWork(); + response.complete(persistence()); + drainWork(); + realm.drain(); + old.complete(persistence()); + drainWork(); + realm.drain(); + + assertEquals(State.READY, storage.getState()); + assertEquals("new", storage.getReadyPreferences().getChatModel()); + verify(files).read(any()); + verify(files, never()).write(any(), any()); + } + + @Test + void testInitialize_SignedOut_DoesNotReadOrResolvePath() { + when(auth.isSignedIn()).thenReturn(false); + storage.initialize(); + storage.retry(); + drainWork(); + realm.drain(); + + assertEquals(State.UNAVAILABLE, storage.getState()); + assertEquals(State.UNAVAILABLE, storage.getReadiness().getValue()); + assertNull(storage.getReadyPreferences()); + verifyNoInteractions(connection, files); + } + + @Test + void testInitialize_BlankAccount_DoesNotResolvePath() { + when(auth.getUserName()).thenReturn(" "); + storage.initialize(); + drainWork(); + realm.drain(); + + assertEquals(State.UNAVAILABLE, storage.getState()); + verifyNoInteractions(connection, files); + } + + @Test + void testAccountChange_ReadyPreferences_AreInaccessibleBeforeRealmCallback() throws Exception { + when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"alice-model\"}"); + load(); + when(auth.getUserName()).thenReturn("bob"); + + assertNull(storage.getReadyPreferences()); + assertEquals(State.UNAVAILABLE, storage.getState()); + storage.persist(); + realm.drain(); + assertEquals(State.UNAVAILABLE, storage.getReadiness().getValue()); + verify(files, never()).write(any(), any()); + verify(connection).persistence(); + } + + @Test + void testAccountChange_QueuedRestoration_CannotPublishOldPreferences() throws Exception { + when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"alice-model\"}"); + storage.initialize(); + drainWork(); + response.complete(persistence()); + drainWork(); + when(auth.getUserName()).thenReturn("bob"); + realm.drain(); + + assertEquals(State.UNAVAILABLE, storage.getReadiness().getValue()); + assertNull(storage.getReadyPreferences()); + verify(connection).persistence(); + } + + @Test + void testAccountChange_LateOldRpc_DoesNotAffectNewAccount() throws Exception { + response = new UncancellableFuture(); + CompletableFuture old = response; + storage.initialize(); + drainWork(); + when(auth.getUserName()).thenReturn("bob"); + authListener().onDidCopilotStatusChange(new CopilotStatusResult()); + realm.drain(); + assertEquals(State.UNAVAILABLE, storage.getState()); + verify(connection).persistence(); + + response = new CompletableFuture<>(); + when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"bob-model\"}"); + storage.retry(); + drainWork(); + response.complete(persistence()); + drainWork(); + realm.drain(); + old.complete(persistence()); + drainWork(); + realm.drain(); + + assertEquals("bob-model", storage.getReadyPreferences().getChatModel()); + verify(files).read(Path.of(persistence().getPath(), "bob", "pref.json")); + verify(files, never()).read(Path.of(persistence().getPath(), "alice", "pref.json")); + } + + @Test + void testAuthNotification_SignOut_InvalidatesWithoutAutomaticRetry() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + when(auth.isSignedIn()).thenReturn(false); + authListener().onDidCopilotStatusChange(new CopilotStatusResult()); + assertNull(storage.getReadyPreferences()); + realm.drain(); + assertEquals(State.UNAVAILABLE, storage.getReadiness().getValue()); + + when(auth.isSignedIn()).thenReturn(true); + authListener().onDidCopilotStatusChange(new CopilotStatusResult()); + drainWork(); + realm.drain(); + verify(connection).persistence(); + assertEquals(State.UNAVAILABLE, storage.getState()); + } + + @Test + void testAuthNotification_SameAccount_DoesNotResetReadySessionChoices() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + UserPreference restored = storage.getReadyPreferences(); + restored.setChatModel("session-choice"); + authListener().onDidCopilotStatusChange(new CopilotStatusResult()); + drainWork(); + realm.drain(); + + assertSame(restored, storage.getReadyPreferences()); + assertEquals("session-choice", storage.getReadyPreferences().getChatModel()); + verify(connection).persistence(); + } + + @Test + void testDispose_PendingRpc_DetachesListenerAndRejectsLateResults() throws Exception { + response = new UncancellableFuture(); + storage.initialize(); + drainWork(); + CopilotAuthStatusListener listener = authListener(); + storage.dispose(); + storage.dispose(); + storage.initialize(); + storage.retry(); + response.complete(persistence()); + drainWork(); + realm.drain(); + + assertEquals(State.DISPOSED, storage.getState()); + assertNull(storage.getReadyPreferences()); + assertTrue(storage.getReadiness().isDisposed()); + verify(auth).removeCopilotAuthStatusListener(listener); + verify(worker).shutdownNow(); + verify(timer).shutdownNow(); + verify(connection).persistence(); + verifyNoInteractions(files); + } + + @Test + void testDispose_QueuedRestoration_DoesNotPublishReady() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + storage.initialize(); + drainWork(); + response.complete(persistence()); + drainWork(); + storage.dispose(); + realm.drain(); + + assertEquals(State.DISPOSED, storage.getState()); + assertNull(storage.getReadyPreferences()); + assertTrue(storage.getReadiness().isDisposed()); + } + + @Test + void testPersist_ReadyPreferences_UsesResolvedAccountPathWithoutRpc() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + storage.getReadyPreferences().setChatModel("saved-model"); + storage.persist(); + + ArgumentCaptor saved = ArgumentCaptor.forClass(String.class); + verify(files).write(eq(Path.of(persistence().getPath(), "alice", "pref.json")), saved.capture()); + assertTrue(saved.getValue().contains("\"chatModel\":\"saved-model\"")); + verify(connection).persistence(); + } + + @Test + void testPersist_ObsoleteAccountPreference_CannotSaveNewAccount() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + UserPreference original = storage.getReadyPreferences(); + when(auth.getUserName()).thenReturn("bob"); + response = new CompletableFuture<>(); + load(); + + storage.persist(original); + verify(files, never()).write(any(), any()); + storage.getReadyPreferences().setChatModel("bob-model"); + storage.persist(storage.getReadyPreferences()); + + ArgumentCaptor saved = ArgumentCaptor.forClass(String.class); + verify(files).write(eq(Path.of(persistence().getPath(), "bob", "pref.json")), saved.capture()); + assertTrue(saved.getValue().contains("\"chatModel\":\"bob-model\"")); + verify(connection, times(2)).persistence(); + } + + @Test + void testPersist_EqualButNotAuthoritativePreference_DoesNotWrite() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + UserPreference unrelated = new UserPreference(); + assertEquals(storage.getReadyPreferences(), unrelated); + + storage.persist(unrelated); + storage.persist(null); + + verify(files, never()).write(any(), any()); + } + + @Test + void testPersist_DisposedPreference_DoesNotWrite() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + UserPreference original = storage.getReadyPreferences(); + storage.dispose(); + + storage.persist(original); + storage.persist(); + + verify(files, never()).write(any(), any()); + } + + @Test + void testPersist_DuringFileWrite_GettersRemainAvailableToOtherThreads() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + UserPreference original = storage.getReadyPreferences(); + Mockito.doAnswer(invocation -> { + assertEquals(State.READY, CompletableFuture.supplyAsync(storage::getState).get(5, TimeUnit.SECONDS)); + assertSame(original, CompletableFuture.supplyAsync(storage::getReadyPreferences).get(5, TimeUnit.SECONDS)); + return null; + }).when(files).write(any(), any()); + + storage.persist(original); + + verify(files).write(any(), any()); + } + + @Test + void testPersist_AccountChangesDuringWrite_UsesOnlyCapturedAccountPath() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + Mockito.doAnswer(invocation -> { + when(auth.getUserName()).thenReturn("bob"); + assertNull(storage.getReadyPreferences()); + return null; + }).when(files).write(any(), any()); + storage.persist(); + + verify(files).write(eq(Path.of(persistence().getPath(), "alice", "pref.json")), any()); + assertEquals(State.UNAVAILABLE, storage.getState()); + verify(connection).persistence(); + } + + @Test + void testPersist_WriteFailure_PreservesReadySessionChoices() throws Exception { + when(files.read(any(Path.class))).thenReturn("{}"); + load(); + storage.getReadyPreferences().setChatModel("session-choice"); + Mockito.doThrow(new IOException("read-only")).when(files).write(any(), any()); + storage.persist(); + + assertEquals(State.READY, storage.getState()); + assertEquals("session-choice", storage.getReadyPreferences().getChatModel()); + } + + @Test + void testPersist_StillLoading_DoesNotWriteDefaultsOrResolveAgain() throws Exception { + storage.initialize(); + drainWork(); + storage.persist(); + + assertEquals(State.LOADING, storage.getState()); + verifyNoInteractions(files); + verify(connection).persistence(); + } + + private CopilotAuthStatusListener authListener() { + ArgumentCaptor listener = ArgumentCaptor.forClass(CopilotAuthStatusListener.class); + verify(auth).addCopilotAuthStatusListener(listener.capture()); + return listener.getValue(); + } + + private void expire() { + clock.addAndGet(TimeUnit.SECONDS.toNanos(15)); + deadlines.remove().run(); + } + + private void load() { + storage.initialize(); + drainWork(); + response.complete(persistence()); + drainWork(); + realm.drain(); + } + + private void assertFailedWithoutWrites() throws IOException { + assertEquals(State.FAILED, storage.getState()); + assertEquals(State.FAILED, storage.getReadiness().getValue()); + assertNull(storage.getReadyPreferences()); + storage.persist(); + verify(files, never()).write(any(), any()); + } + + private ChatPersistence persistence() { + ChatPersistence result = new ChatPersistence(); + result.setPath(Path.of("preferences").toAbsolutePath().toString()); + return result; + } + + private void drainWork() { + while (!work.isEmpty()) { + work.remove().run(); + } + } + + private static class TestRealm extends Realm { + private final Queue publications = new ConcurrentLinkedQueue<>(); + + @Override + public boolean isCurrent() { + return true; + } + + @Override + public void asyncExec(Runnable runnable) { + publications.add(runnable); + } + + void drain() { + while (!publications.isEmpty()) { + publications.remove().run(); + } + } + } + + private static class UncancellableFuture extends CompletableFuture { + @Override + public boolean cancel(boolean mayInterruptIfRunning) { + return false; + } + } + + // A public concrete boundary adapter supports Mockito's inline mock maker across OSGi class loaders. + public static class TestFileAccess implements PreferenceStorage.FileAccess { + @Override + public String read(Path path) throws IOException { + return Files.readString(path); + } + + @Override + public void write(Path path, String content) throws IOException { + Files.writeString(path, content); + } + } +} 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 7b36a797a..5c07a19a5 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 @@ -4,180 +4,260 @@ package com.microsoft.copilot.eclipse.ui.chat.services; 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.assertTrue; +import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; - -import java.lang.reflect.Field; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import org.eclipse.e4.core.services.events.IEventBroker; +import static org.mockito.Mockito.mock; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Arrays; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.BooleanSupplier; + +import org.eclipse.swt.SWT; +import org.eclipse.jface.databinding.swt.DisplayRealm; +import org.eclipse.swt.widgets.Display; +import org.eclipse.swt.widgets.Shell; +import org.eclipse.swt.widgets.Label; +import org.eclipse.swt.widgets.Link; import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.io.TempDir; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; -import org.osgi.service.event.Event; -import org.osgi.service.event.EventHandler; import com.microsoft.copilot.eclipse.core.AuthStatusManager; -import com.microsoft.copilot.eclipse.core.chat.InputNavigation; -import com.microsoft.copilot.eclipse.core.events.CopilotEventConstants; +import com.microsoft.copilot.eclipse.core.CopilotAuthStatusListener; +import com.microsoft.copilot.eclipse.core.CopilotCore; +import com.microsoft.copilot.eclipse.core.FeatureFlags; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatMode; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult; +import com.microsoft.copilot.eclipse.ui.swt.DropdownButton; +import com.microsoft.copilot.eclipse.ui.chat.Messages; +import com.microsoft.copilot.eclipse.ui.chat.PreferenceStatus; @ExtendWith(MockitoExtension.class) class UserPreferenceServiceTest { - @Mock - private CopilotLanguageServerConnection mockLsConnection; - + private CopilotLanguageServerConnection connection; @Mock - private AuthStatusManager mockAuthStatusManager; + private AuthStatusManager auth; + @TempDir + private Path directory; - private UserPreferenceService userPreferenceService; - - @BeforeEach - void setUp() { - when(mockAuthStatusManager.isSignedIn()).thenReturn(false); - } + private PreferenceStorage storage; + private UserPreferenceService service; + private Shell shell; + private DropdownButton picker; + private PreferenceStatus status; @AfterEach void tearDown() { - if (userPreferenceService != null) { - userPreferenceService.dispose(); - } + Display.getDefault().syncExec(() -> { + if (shell != null) { + shell.dispose(); + } + if (service != null) { + service.dispose(); + } + if (storage != null) { + storage.dispose(); + } + }); } @Test - void testAuthStatusChangedEventHandler_UserSignsOut_ClearsUserPreferenceCache() { - // Arrange - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); - - // Set up initial state with input navigation - setInputNavigationForService(new InputNavigation()); - assertNotNull(getInputNavigationFromService(), "Input navigation should be set initially"); - - // Get the auth status changed event handler - EventHandler authHandler = getAuthStatusChangedEventHandler(); - assertNotNull(authHandler, "Auth status changed event handler should be available"); - - Event signOutEvent = createAuthStatusEvent(CopilotStatusResult.NOT_SIGNED_IN); - - // Act - authHandler.handleEvent(signOutEvent); - - // Assert - assertNull(getInputNavigationFromService(), "Input navigation should be cleared when user signs out"); + void testInitialization_RestoresModeHistoryAndConfirmationWithoutSaving() throws Exception { + String saved = """ + {"chatModeName":"Ask","userInputs":["first","second"],"skipGitHubJobConfirmDialog":true} + """; + Path file = writePreferences(saved); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> "Ask".equals(service.getActiveModeNameOrId())); + Display.getDefault().syncExec(() -> { + assertEquals(ChatMode.Ask, service.getActiveChatMode()); + assertEquals("second", service.getPreviousInput("")); + assertEquals("first", service.getPreviousInput("")); + assertTrue(service.isSkipGitHubJobConfirmDialog()); + assertTrue(picker.getEnabled()); + assertFalse(status.getVisible()); + }); + assertEquals(saved, Files.readString(file), "Restoration is not a user edit"); } @Test - void testAuthStatusChangedEventHandler_SignOutThenSignIn() { - // Arrange - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); - - EventHandler authHandler = getAuthStatusChangedEventHandler(); - assertNotNull(authHandler, "Auth status changed event handler should be available"); - - Event signOutEvent = createAuthStatusEvent(CopilotStatusResult.NOT_SIGNED_IN); - Event signInEvent = createAuthStatusEvent(CopilotStatusResult.OK, "test-user"); - - // Act - Sign out then sign in - authHandler.handleEvent(signOutEvent); - assertNull(getInputNavigationFromService(), "Input navigation should be null after sign out"); - - authHandler.handleEvent(signInEvent); - InputNavigation initialNavigation = new InputNavigation(List.of("input1", "input2")); - setInputNavigationForService(initialNavigation); - assertNotNull(getInputNavigationFromService(), "Input navigation should be set initially"); - - // 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"); + void testInitialization_AgentPolicyDisabled_RestoresAskView() throws Exception { + writePreferences("{\"chatModeName\":\"Agent\"}"); + FeatureFlags flags = CopilotCore.getPlugin().getFeatureFlags(); + boolean original = flags.isAgentModeEnabled(); + try { + flags.setAgentModeEnabled(false); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> "Agent".equals(service.getActiveModeNameOrId())); + Display.getDefault().syncExec(() -> assertEquals(ChatMode.Ask, service.getActiveChatMode())); + } finally { + flags.setAgentModeEnabled(original); + } } @Test - void testAuthStatusChangedEventHandler_UserSignsIn_ReloadsBuiltInModes() { - // Arrange - userPreferenceService = new UserPreferenceService(mockLsConnection, mockAuthStatusManager); - - EventHandler authHandler = getAuthStatusChangedEventHandler(); - assertNotNull(authHandler, "Auth status changed event handler should be available"); - - Event signInEvent = createAuthStatusEvent(CopilotStatusResult.OK, "test-user"); + void testRetry_FailedLoadKeepsPickerDisabledThenRestoresSavedChoice() throws Exception { + writePreferences("{\"chatModeName\":\"Ask\"}"); + startAuthenticated(CompletableFuture.failedFuture(new IllegalStateException("offline"))); + awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.FAILED); + Display.getDefault().syncExec(() -> { + assertFalse(picker.getEnabled()); + assertTrue(status.getVisible()); + Label message = Arrays.stream(status.getChildren()).filter(Label.class::isInstance) + .map(Label.class::cast).findFirst().orElseThrow(); + assertEquals(Messages.preferenceLoadFailed, message.getText()); + service.setActiveChatMode("Agent"); + assertNull(service.getActiveModeNameOrId()); + }); + when(connection.persistence()).thenReturn(CompletableFuture.completedFuture(persistence())); + Display.getDefault().syncExec(() -> { + Link retry = Arrays.stream(status.getChildren()).filter(Link.class::isInstance) + .map(Link.class::cast).findFirst().orElseThrow(); + assertTrue(retry.getEnabled()); + retry.notifyListeners(SWT.Selection, new org.eclipse.swt.widgets.Event()); + }); + awaitUi(() -> picker.getEnabled() && "Ask".equals(service.getActiveModeNameOrId())); + } - // Act - Simulate user sign in - authHandler.handleEvent(signInEvent); + @Test + void testSignOut_InvalidatesLoadedHistoryAndDisablesPicker() throws Exception { + writePreferences("{\"chatModeName\":\"Ask\",\"userInputs\":[\"private history\"]}"); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> "Ask".equals(service.getActiveModeNameOrId())); + ArgumentCaptor listener = ArgumentCaptor.forClass(CopilotAuthStatusListener.class); + verify(auth).addCopilotAuthStatusListener(listener.capture()); + when(auth.isSignedIn()).thenReturn(false); + CopilotStatusResult signedOut = new CopilotStatusResult(); + signedOut.setStatus(CopilotStatusResult.NOT_SIGNED_IN); + listener.getValue().onDidCopilotStatusChange(signedOut); + awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.UNAVAILABLE); + Display.getDefault().syncExec(() -> { + assertFalse(picker.getEnabled()); + assertNull(service.getActiveModeNameOrId()); + assertEquals("", service.getPreviousInput("")); + }); + } - // 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"); + @Test + void testModeChange_AfterRestoration_UpdatesSharedStateAndPersists() throws Exception { + writePreferences("{\"chatModeName\":\"Ask\"}"); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> "Ask".equals(service.getActiveModeNameOrId())); + Display.getDefault().syncExec(() -> { + service.setActiveChatMode("Agent"); + assertEquals("Agent", service.getActiveModeNameOrId()); + assertNotNull(storage.getReadyPreferences()); + assertEquals("Agent", storage.getReadyPreferences().getChatModeName()); + }); + assertTrue(Files.readString(directory.resolve("user").resolve("pref.json")).contains("Agent")); } - /** - * Helper method to create an auth status changed event - */ - private Event createAuthStatusEvent(String status) { - return createAuthStatusEvent(status, null); + @Test + void testInitialization_DeadlineExpires_ShowsFailureAndRetryWithoutEnablingPicker() throws Exception { + when(auth.isSignedIn()).thenReturn(true); + when(auth.getUserName()).thenReturn("user"); + CompletableFuture pending = new CompletableFuture<>(); + when(connection.persistence()).thenReturn(pending); + ScheduledExecutorService timer = mock(ScheduledExecutorService.class); + AtomicReference deadline = new AtomicReference<>(); + when(timer.schedule(any(Runnable.class), eq(15L), eq(TimeUnit.SECONDS))).thenAnswer(invocation -> { + deadline.set(invocation.getArgument(0)); + return mock(ScheduledFuture.class); + }); + AtomicLong clock = new AtomicLong(); + Display.getDefault().syncExec(() -> { + storage = new PreferenceStorage(connection, auth, DisplayRealm.getRealm(Display.getDefault()), + Executors.newSingleThreadExecutor(), timer, clock::get, new PreferenceStorage.FileAccess() { + @Override + public String read(Path path) throws IOException { + return Files.readString(path); + } + + @Override + public void write(Path path, String content) throws IOException { + Files.writeString(path, content); + } + }); + createControls(); + }); + awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.LOADING); + Display.getDefault().syncExec(() -> assertFalse(picker.getEnabled())); + clock.set(TimeUnit.SECONDS.toNanos(15)); + deadline.get().run(); + awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.FAILED); + Display.getDefault().syncExec(() -> { + assertFalse(picker.getEnabled()); + assertTrue(status.getVisible()); + Link retry = Arrays.stream(status.getChildren()).filter(Link.class::isInstance) + .map(Link.class::cast).findFirst().orElseThrow(); + assertTrue(retry.getEnabled()); + }); } - /** - * Helper method to create an auth status changed event with user - */ - private Event createAuthStatusEvent(String status, String user) { - CopilotStatusResult statusResult = new CopilotStatusResult(); - statusResult.setStatus(status); - if (user != null) { - statusResult.setUser(user); - } + private void startAuthenticated(CompletableFuture rpc) { + when(auth.isSignedIn()).thenReturn(true); + when(auth.getUserName()).thenReturn("user"); + when(connection.persistence()).thenReturn(rpc); + Display.getDefault().syncExec(() -> { + storage = new PreferenceStorage(connection, auth); + createControls(); + }); + } - Map eventProperties = new HashMap<>(); - eventProperties.put(IEventBroker.DATA, statusResult); - return new Event(CopilotEventConstants.TOPIC_AUTH_STATUS_CHANGED, eventProperties); + private void createControls() { + service = new UserPreferenceService(connection, auth, storage); + shell = new Shell(Display.getDefault()); + status = new PreferenceStatus(shell, storage); + picker = new DropdownButton(shell, SWT.NONE); + service.bindChatModePicker(picker); } - /** - * Helper method to access private authStatusChangedEventHandler field for - * testing - */ - private EventHandler getAuthStatusChangedEventHandler() { - try { - Field field = UserPreferenceService.class.getDeclaredField("authStatusChangedEventHandler"); - field.setAccessible(true); - return (EventHandler) field.get(userPreferenceService); - } catch (Exception e) { - throw new RuntimeException("Failed to access authStatusChangedEventHandler field", e); - } + private ChatPersistence persistence() { + ChatPersistence result = new ChatPersistence(); + result.setPath(directory.toString()); + return result; } - /** - * Helper method to access private inputNavigation field for testing - */ - private InputNavigation getInputNavigationFromService() { - try { - Field field = UserPreferenceService.class.getDeclaredField("inputNavigation"); - field.setAccessible(true); - return (InputNavigation) field.get(userPreferenceService); - } catch (Exception e) { - throw new RuntimeException("Failed to access inputNavigation field", e); - } + private Path writePreferences(String content) throws Exception { + Path file = directory.resolve("user").resolve("pref.json"); + Files.createDirectories(file.getParent()); + Files.writeString(file, content); + return file; } - /** - * Helper method to set private inputNavigation field for testing - */ - private void setInputNavigationForService(InputNavigation inputNavigation) { - try { - Field field = UserPreferenceService.class.getDeclaredField("inputNavigation"); - field.setAccessible(true); - field.set(userPreferenceService, inputNavigation); - } catch (Exception e) { - throw new RuntimeException("Failed to set inputNavigation field", e); - } + private static void awaitUi(BooleanSupplier condition) throws InterruptedException { + AtomicBoolean complete = new AtomicBoolean(); + long deadline = System.nanoTime() + java.util.concurrent.TimeUnit.SECONDS.toNanos(5); + do { + Display.getDefault().syncExec(() -> complete.set(condition.getAsBoolean())); + if (complete.get()) { + return; + } + Thread.sleep(10); + } while (System.nanoTime() < deadline); + assertTrue(complete.get(), "Timed out waiting for the preference UI"); } -} \ No newline at end of file +} diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java index 43930884d..09d2386ed 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java @@ -14,6 +14,8 @@ import java.util.UUID; import org.apache.commons.lang3.StringUtils; +import org.eclipse.core.databinding.observable.Realm; +import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; import org.eclipse.core.resources.IContainer; import org.eclipse.core.resources.IFile; import org.eclipse.core.resources.IFolder; @@ -76,6 +78,7 @@ import com.microsoft.copilot.eclipse.ui.chat.contextwindow.ContextSizeDonut; import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager; import com.microsoft.copilot.eclipse.ui.chat.services.ModelService; +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage; import com.microsoft.copilot.eclipse.ui.chat.services.ReferencedFileService; import com.microsoft.copilot.eclipse.ui.chat.services.UserPreferenceService; import com.microsoft.copilot.eclipse.ui.chat.tools.JavaDebuggerToolAdapter; @@ -145,6 +148,7 @@ public ActionBar(Composite parent, int style, ChatServiceManager chatServiceMana this.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false)); this.setData(CssConstants.CSS_ID_KEY, "chat-action-bar-wrapper"); this.chatServiceManager = chatServiceManager; + new PreferenceStatus(this, chatServiceManager.getPreferenceStorage()); this.updateSendButtonToCancelButtonHandler = event -> { updateButtonState(SendOrCancelButtonStates.CANCEL_ENABLED); }; @@ -337,6 +341,22 @@ private void updateTableLayout(Table table) { // Update send to job button and send button together updateButtonsLayout(); + PreferenceStorage storage = chatServiceManager.getPreferenceStorage(); + Realm.runWithDefault(storage.getReadiness().getRealm(), () -> { + ISideEffect readinessEffect = ISideEffect.create(storage.getReadiness()::getValue, state -> { + if (isDisposed()) { + return; + } + boolean ready = state == PreferenceStorage.State.READY; + mcpToolButton.setEnabled(ready); + autoBreakpointButton.setEnabled(ready); + if (isSendButton) { + updateButtonState(StringUtils.isBlank(inputTextViewer.getContent()) + ? SendOrCancelButtonStates.SEND_DISABLED : SendOrCancelButtonStates.SEND_ENABLED); + } + }); + addDisposeListener(event -> readinessEffect.dispose()); + }); } /** @@ -433,7 +453,7 @@ public void updateButtonsLayout() { this.sendToJobButton = UiUtils.createIconButton(this.bottomRightButtonsComposite, SWT.PUSH | SWT.FLAT); boolean hasText = !StringUtils.isBlank(this.inputTextViewer.getContent()); - this.sendToJobButton.setEnabled(hasText); + this.sendToJobButton.setEnabled(hasText && isSendButton && preferencesReady()); this.sendToJobButton.setImage(hasText ? sendToJobImage : sendToJobDisabledImage); this.sendToJobButton.setToolTipText(Messages.chat_actionBar_sendToJobButton_Tooltip); AccessibilityUtils.addAccessibilityNameForUiComponent(this.sendToJobButton, @@ -457,7 +477,7 @@ public void widgetSelected(SelectionEvent e) { this.sendDisabledImage = CopilotImages.getImage(CopilotImages.IMG_CHAT_SEND_DISABLED); this.btnMsgToggle = UiUtils.createIconButton(bottomRightButtonsComposite, SWT.PUSH | SWT.FLAT); boolean isEnabled = !StringUtils.isBlank(this.inputTextViewer.getContent()); - this.btnMsgToggle.setEnabled(isEnabled); + this.btnMsgToggle.setEnabled(isEnabled && preferencesReady()); this.btnMsgToggle.setImage(isEnabled ? this.sendImage : this.sendDisabledImage); this.btnMsgToggle.setToolTipText(Messages.chat_actionBar_sendButton_Tooltip); GridData sendGd = new GridData(SWT.RIGHT, SWT.CENTER, false, false); @@ -477,6 +497,9 @@ public void widgetSelected(org.eclipse.swt.events.SelectionEvent e) { AccessibilityUtils.addAccessibilityNameForUiComponent(this.btnMsgToggle, Messages.chat_actionBar_sendButton_Tooltip); } + if (!isSendButton) { + updateButtonState(SendOrCancelButtonStates.CANCEL_ENABLED); + } // Refresh the layout this.bottomRightButtonsComposite.requestLayout(); } @@ -762,6 +785,10 @@ public void setInputTextViewerContent(String content) { * Handles the send message event. */ public void handleSendMessage() { + if (!preferencesReady()) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot send chat before preferences are ready")); + return; + } updateButtonState(SendOrCancelButtonStates.CANCEL_ENABLED); String message = this.inputTextViewer.getContent(); String workDoneToken = UUID.randomUUID().toString(); @@ -773,6 +800,10 @@ public void handleSendMessage() { * Handles the send to job button click event. Shows a dialog to inform user about git repository requirement. */ private void handleSendToJob() { + if (!preferencesReady()) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot send a job before preferences are ready")); + return; + } ChatServiceManager chatServiceManager = (ChatServiceManager) CopilotCore.getPlugin().getChatServiceManager(); if (chatServiceManager != null) { UserPreferenceService userPreferenceService = chatServiceManager.getUserPreferenceService(); @@ -853,6 +884,9 @@ public boolean isSendButton() { } private void updateButtonState(SendOrCancelButtonStates state) { + if (state == SendOrCancelButtonStates.SEND_ENABLED && !preferencesReady()) { + state = SendOrCancelButtonStates.SEND_DISABLED; + } switch (state) { case SEND_ENABLED: isSendButton = true; @@ -873,6 +907,11 @@ private void updateButtonState(SendOrCancelButtonStates state) { default: break; } + + } + + private boolean preferencesReady() { + return chatServiceManager.getPreferenceStorage().getState() == PreferenceStorage.State.READY; } /** diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java index 29b8eb3d4..7db63670f 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java @@ -90,6 +90,7 @@ import com.microsoft.copilot.eclipse.ui.chat.services.AgentToolService; import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager; import com.microsoft.copilot.eclipse.ui.chat.services.DebugEventAutoResponseHandler; +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage; import com.microsoft.copilot.eclipse.ui.chat.services.ReferencedFileService; import com.microsoft.copilot.eclipse.ui.chat.services.TodoListService; import com.microsoft.copilot.eclipse.ui.chat.viewers.AfterLoginWelcomeViewer; @@ -1029,6 +1030,13 @@ public void setFocus() { private void onSendInternal(String workDoneToken, String message, String agentSlug, String agentJobWorkspaceFolder, boolean createNewTurn) { + if (chatServiceManager.getPreferenceStorage().getState() != PreferenceStorage.State.READY) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot send chat before preferences are ready")); + if (actionBar != null && !actionBar.isDisposed()) { + actionBar.resetSendButton(); + } + return; + } // Persist the user input to history chatServiceManager.getUserPreferenceService().addInputToHistory(message); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/HandoffContainer.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/HandoffContainer.java index 6d986e0c7..b943ca428 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/HandoffContainer.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/HandoffContainer.java @@ -7,6 +7,8 @@ import java.util.ArrayList; import java.util.List; +import org.eclipse.core.databinding.observable.Realm; +import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; import org.eclipse.swt.SWT; import org.eclipse.swt.layout.GridData; import org.eclipse.swt.layout.GridLayout; @@ -22,6 +24,7 @@ import com.microsoft.copilot.eclipse.core.lsp.protocol.ConversationMode.HandOff; import com.microsoft.copilot.eclipse.ui.chat.services.ChatFontService; import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager; +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage; import com.microsoft.copilot.eclipse.ui.swt.CssConstants; /** @@ -60,6 +63,15 @@ public HandoffContainer(Composite parent, ChatServiceManager chatServiceManager, // Initially hidden this.setVisible(false); ((GridData) this.getLayoutData()).exclude = true; + PreferenceStorage storage = chatServiceManager.getPreferenceStorage(); + Realm.runWithDefault(storage.getReadiness().getRealm(), () -> { + ISideEffect effect = ISideEffect.create(storage.getReadiness()::getValue, state -> { + if (!isDisposed() && state != PreferenceStorage.State.READY) { + hide(); + } + }); + addDisposeListener(event -> effect.dispose()); + }); } /** @@ -78,6 +90,10 @@ public void hide() { * Show handoff buttons based on the current mode and update their content. */ public void show() { + if (chatServiceManager.getPreferenceStorage().getState() != PreferenceStorage.State.READY) { + hide(); + return; + } // Clear existing buttons clearHandoffs(); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java index bb946f7a3..cdec33180 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java @@ -21,6 +21,10 @@ public final class Messages extends NLS { public static String chat_warnWidget_defaultErrorMsg; public static String chat_warnWidget_byokQuotaUsageMessage; public static String configureModes; + public static String preferenceLoading; + public static String preferenceLoadFailed; + public static String preferenceUnavailable; + public static String preferenceRetry; public static String agentMessageWidget_openInBrowserButton; public static String agentMessageWidget_openInBrowserTooltip; public static String agentMessageWidget_openJobListButton; diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java new file mode 100644 index 000000000..ad069e55c --- /dev/null +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java @@ -0,0 +1,64 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.ui.chat; + +import org.eclipse.core.databinding.observable.Realm; +import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; +import org.eclipse.swt.SWT; +import org.eclipse.swt.layout.GridData; +import org.eclipse.swt.layout.GridLayout; +import org.eclipse.swt.widgets.Composite; +import org.eclipse.swt.widgets.Label; +import org.eclipse.swt.widgets.Link; + +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage; +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage.State; + +/** + * Inline preference-loading status, independent of model and conversation loading. + */ +public class PreferenceStatus extends Composite { + /** + * Creates a status message and an explicit retry action. + * + * @param parent parent control + * @param storage shared chat preference storage + */ + public PreferenceStatus(Composite parent, PreferenceStorage storage) { + super(parent, SWT.NONE); + setLayout(new GridLayout(2, false)); + GridData data = new GridData(SWT.FILL, SWT.CENTER, true, false); + setLayoutData(data); + Label message = new Label(this, SWT.WRAP); + message.setLayoutData(new GridData(SWT.FILL, SWT.CENTER, true, false)); + message.setData("org.eclipse.swtbot.widget.key", "preference-status"); + Link retry = new Link(this, SWT.NONE); + retry.setText("" + Messages.preferenceRetry + ""); + retry.setData("org.eclipse.swtbot.widget.key", "preference-retry"); + GridData retryData = new GridData(SWT.RIGHT, SWT.CENTER, false, false); + retry.setLayoutData(retryData); + retry.addListener(SWT.Selection, event -> storage.retry()); + Realm.runWithDefault(storage.getReadiness().getRealm(), () -> { + ISideEffect effect = ISideEffect.create(storage.getReadiness()::getValue, state -> { + if (isDisposed()) { + return; + } + boolean visible = state != State.READY && state != State.DISPOSED; + data.exclude = !visible; + setVisible(visible); + message.setText(switch (state) { + case LOADING -> Messages.preferenceLoading; + case FAILED -> Messages.preferenceLoadFailed; + default -> Messages.preferenceUnavailable; + }); + boolean canRetry = state == State.FAILED || state == State.UNAVAILABLE; + retryData.exclude = !canRetry; + retry.setVisible(canRetry); + retry.setEnabled(canRetry); + requestLayout(); + }); + addDisposeListener(event -> effect.dispose()); + }); + } +} diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties index 671b22f88..9e0222df8 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties @@ -77,3 +77,8 @@ confirmation_message_fileOperation=The model wants to access sensitive file ({0} # Misc confirmation_autoApprovedDescription=Auto-approved from dialog +# Chat preference loading +preferenceLoading=Loading chat preferences... +preferenceLoadFailed=Could not load chat preferences. Retry to restore your saved settings. +preferenceUnavailable=Chat preferences are unavailable. +preferenceRetry=Retry diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java index 0ad576304..b87636a2c 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ChatBaseService.java @@ -3,40 +3,22 @@ package com.microsoft.copilot.eclipse.ui.chat.services; -import java.nio.file.Path; -import java.nio.file.Paths; -import java.util.concurrent.ExecutionException; - -import com.google.gson.Gson; -import com.google.gson.JsonSyntaxException; -import org.apache.commons.lang3.StringUtils; import org.eclipse.core.databinding.observable.Realm; import org.eclipse.core.databinding.observable.value.IObservableValue; -import org.eclipse.jdt.annotation.Nullable; import org.eclipse.jface.databinding.swt.DisplayRealm; import org.eclipse.swt.widgets.Display; import com.microsoft.copilot.eclipse.core.AuthStatusManager; -import com.microsoft.copilot.eclipse.core.CopilotCore; -import com.microsoft.copilot.eclipse.core.chat.UserPreference; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; -import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; -import com.microsoft.copilot.eclipse.core.utils.PlatformUtils; import com.microsoft.copilot.eclipse.ui.utils.SwtUtils; /** * Base class for chat services. */ public abstract class ChatBaseService { - protected static final Gson gson = new Gson(); - protected static final String PREF_FILE_NAME = "pref.json"; - protected CopilotLanguageServerConnection lsConnection; protected AuthStatusManager authStatusManager; - protected String persistentPath; - private static UserPreference userPreference; - /** * Constructor for the ChatBaseService. */ @@ -45,53 +27,6 @@ protected ChatBaseService(CopilotLanguageServerConnection lsConnection, AuthStat this.authStatusManager = authStatusManager; } - /** - * Get User Preference. - */ - protected synchronized UserPreference getUserPreference() { - if (userPreference != null) { - return userPreference; - } - - Path path = getPersistentFilePath(); - if (path != null) { - try { - String jsonContent = PlatformUtils.readFileContent(path); - if (!jsonContent.isEmpty()) { - userPreference = gson.fromJson(jsonContent, UserPreference.class); - if (userPreference != null) { - return userPreference; - } - } - } catch (JsonSyntaxException e) { - CopilotCore.LOGGER.error("Failed to get user preference, will generate a new one.", e); - } - } - - userPreference = new UserPreference(); - return userPreference; - } - - /** - * Clear User Preference. - */ - protected void clearUserPreferenceCache() { - userPreference = null; - } - - /** - * Persist User Preference. - */ - public synchronized void persistUserPreference() { - Path path = getPersistentFilePath(); - if (path == null) { - return; - } - - String jsonContent = gson.toJson(userPreference); - PlatformUtils.writeFileContent(path, jsonContent); - } - /** * Ensures operations run in the correct Realm. * @@ -113,32 +48,6 @@ protected void ensureRealm(Runnable runnable) { } } - /** - * Get the path for the persistent file. - */ - private @Nullable Path getPersistentFilePath() { - if (!this.authStatusManager.isSignedIn()) { - return null; - } - if (this.persistentPath == null) { - try { - ChatPersistence chatPersistence = this.lsConnection.persistence().get(); - this.persistentPath = chatPersistence.getPath(); - } catch (InterruptedException | ExecutionException e) { - CopilotCore.LOGGER.error("Failed to get persistent path", e); - return null; - } - } - - final String user = this.authStatusManager.getUserName(); - if (StringUtils.isBlank(user)) { - CopilotCore.LOGGER.error(new IllegalStateException("User name is empty")); - return null; - } - - return Paths.get(this.persistentPath, user, PREF_FILE_NAME); - } - /** * Update the value of an observable in its realm. * @@ -147,7 +56,11 @@ protected void ensureRealm(Runnable runnable) { */ protected void updateObservable(IObservableValue observable, final T value) { if (observable != null) { - observable.getRealm().asyncExec(() -> observable.setValue(value)); + observable.getRealm().asyncExec(() -> { + if (!observable.isDisposed()) { + observable.setValue(value); + } + }); } } } 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 62e6c4a6a..6f356be96 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 @@ -24,6 +24,7 @@ public class ChatServiceManager implements IChatServiceManager { private ModelService modelService; private ByokService byokService; private UserPreferenceService userPreferenceService; + private PreferenceStorage preferenceStorage; private AvatarService avatarService; private AgentToolService agentToolService; private FileToolService fileToolService; @@ -45,8 +46,9 @@ public ChatServiceManager() { this.lsConnection = CopilotCore.getPlugin().getCopilotLanguageServer(); 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); + preferenceStorage = new PreferenceStorage(this.lsConnection, this.authStatusManager); + userPreferenceService = new UserPreferenceService(this.lsConnection, this.authStatusManager, preferenceStorage); + modelService = new ModelService(this.lsConnection, this.authStatusManager, preferenceStorage); avatarService = new AvatarService(this.authStatusManager); agentToolService = new AgentToolService(this.lsConnection); fileToolService = new FileToolService(this.lsConnection); @@ -112,6 +114,15 @@ public UserPreferenceService getUserPreferenceService() { return userPreferenceService; } + /** + * Returns the lifecycle-owned chat preference storage. + * + * @return shared preference storage + */ + public PreferenceStorage getPreferenceStorage() { + return preferenceStorage; + } + /** * Get the model service. * @@ -208,6 +219,7 @@ public void dispose() { this.chatCompletionService.dispose(); this.modelService.dispose(); this.userPreferenceService.dispose(); + this.preferenceStorage.dispose(); this.agentToolService.dispose(); this.referencedFileService.dispose(); this.mcpConfigService.dispose(); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java index 88bcdf711..7e259d4cd 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java @@ -7,6 +7,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutionException; @@ -50,6 +51,12 @@ * BYOK integration, UI binding, and communicates with other services through pure events. */ public class ModelService extends ChatBaseService { + private final PreferenceStorage preferenceStorage; + private ISideEffect readinessSideEffect; + private volatile boolean disposed; + private long modelGeneration; + private Job modelJob; + private String modelAccount; // models for the model picker private IObservableValue> modelObservable; @@ -79,27 +86,51 @@ public class ModelService extends ChatBaseService { /** * Constructor for the ModelService. */ - public ModelService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager) { + public ModelService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager, + PreferenceStorage preferenceStorage) { super(lsConnection, authStatusManager); + this.preferenceStorage = preferenceStorage; ensureRealm(() -> { modelObservable = new WritableValue<>(new HashMap<>(), HashMap.class); activeModelObservable = new WritableValue<>(null, CopilotModel.class); - Map initialEfforts = Map.of(); - UserPreference initialPreference = getUserPreference(); - if (initialPreference != null) { - initialEfforts = initialPreference.getReasoningEffortSnapshot(); - } - reasoningEffortObservable = new WritableValue<>(initialEfforts, Map.class); - Map initialContextWindows = Map.of(); - if (initialPreference != null) { - initialContextWindows = initialPreference.getContextWindowSnapshot(); - } - contextWindowObservable = new WritableValue<>(initialContextWindows, Map.class); + reasoningEffortObservable = new WritableValue<>(Map.of(), Map.class); + contextWindowObservable = new WritableValue<>(Map.of(), Map.class); + readinessSideEffect = ISideEffect.create(preferenceStorage.getReadiness()::getValue, state -> { + UserPreference preference = getUserPreference(); + if (state == PreferenceStorage.State.READY && preference != null) { + currentChatMode = "Ask".equalsIgnoreCase(preference.getChatModeName()) ? ChatMode.Ask : ChatMode.Agent; + FeatureFlags flags = CopilotCore.getPlugin().getFeatureFlags(); + if (flags != null && !flags.isAgentModeEnabled()) { + currentChatMode = ChatMode.Ask; + } + reasoningEffortObservable.setValue(preference.getReasoningEffortSnapshot()); + contextWindowObservable.setValue(preference.getContextWindowSnapshot()); + reconcileReasoningEfforts(); + reconcileContextWindows(); + updateModelsForChatMode(currentChatMode); + } else { + activeModelObservable.setValue(null); + reasoningEffortObservable.setValue(Map.of()); + contextWindowObservable.setValue(Map.of()); + if (state == PreferenceStorage.State.UNAVAILABLE) { + copilotModels = new HashMap<>(); + registeredByokModels = new HashMap<>(); + defaultModel = null; + fallbackModel = null; + modelObservable.setValue(Map.of()); + modelGeneration++; + if (modelJob != null) { + modelJob.cancel(); + } + } + } + }); }); initializeEventHandlers(); subscribeToEvents(); + preferenceStorage.initialize(); initializeModels(); } @@ -124,10 +155,12 @@ private void initializeEventHandlers() { if (property instanceof Map modelsMap) { @SuppressWarnings("unchecked") Map> byokModels = (Map>) modelsMap; - saveRegisteredByokModels(byokModels); - reconcileReasoningEfforts(); - reconcileContextWindows(); - ensureRealm(() -> updateModelsForChatMode(currentChatMode)); + ensureRealm(() -> { + saveRegisteredByokModels(byokModels); + reconcileReasoningEfforts(); + reconcileContextWindows(); + updateModelsForChatMode(currentChatMode); + }); } }; @@ -183,32 +216,49 @@ private void subscribeToEvents() { } private void initializeModels() { - if (authStatusManager.isSignedIn()) { - Job job = new Job("Fetching all models...") { + ensureRealm(() -> { + if (!authStatusManager.isSignedIn()) { + return; + } + long generation = ++modelGeneration; + String account = authStatusManager.getUserName(); + modelJob = new Job("Fetching all models...") { @Override protected IStatus run(IProgressMonitor monitor) { try { - fetchCopilotModels(); - fetchByokModels(); - reconcileReasoningEfforts(); - reconcileContextWindows(); + CopilotModel[] models = lsConnection.listModels().get(); + ByokListModelResponse byok = lsConnection.listByokModels(new ByokListModelParams(null, false)).get(); ensureRealm(() -> { + if (generation != modelGeneration || !authStatusManager.isSignedIn() + || !Objects.equals(account, authStatusManager.getUserName())) { + return; + } + saveCopilotModels(models); + modelAccount = account; + if (byok != null && byok.getModels() != null) { + saveRegisteredByokModels(byok.getModels().stream() + .collect(java.util.stream.Collectors.groupingBy(ByokModel::getProviderName))); + } + reconcileReasoningEfforts(); + reconcileContextWindows(); updateModelsForChatMode(currentChatMode); }); - } catch (InterruptedException | ExecutionException e) { + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return Status.CANCEL_STATUS; + } catch (ExecutionException e) { CopilotCore.LOGGER.error("Failed to initialize models", e); } return Status.OK_STATUS; } }; - job.setSystem(true); - job.schedule(); - } + modelJob.setSystem(true); + modelJob.schedule(); + }); } - private void fetchCopilotModels() throws InterruptedException, ExecutionException { - CopilotModel[] modelArray = lsConnection.listModels().get(); + private void saveCopilotModels(CopilotModel[] modelArray) { Map newModels = new HashMap<>(); CopilotModel newDefaultModel = null; CopilotModel newFallbackModel = null; @@ -232,15 +282,6 @@ private void fetchCopilotModels() throws InterruptedException, ExecutionExceptio fallbackModel = newFallbackModel; } - private void fetchByokModels() throws InterruptedException, ExecutionException { - ByokListModelResponse response = lsConnection.listByokModels(new ByokListModelParams(null, false)).get(); - if (response != null && response.getModels() != null) { - Map> modelsByProvider = response.getModels().stream() - .collect(java.util.stream.Collectors.groupingBy(ByokModel::getProviderName)); - saveRegisteredByokModels(modelsByProvider); - } - } - private void saveRegisteredByokModels(Map> byokModels) { Map newByokModels = new HashMap<>(); for (List providerModels : byokModels.values()) { @@ -268,15 +309,21 @@ private String restoreActiveModel() { } private void updateModelsForChatMode(ChatMode chatMode) { + if (getUserPreference() == null) { + return; + } String scope = modeToScope(chatMode); // Filter models for the current mode from combined models final Map modelsForCurrentMode = new HashMap<>(); Map allModels = new HashMap<>(); - allModels.putAll(copilotModels); + boolean currentAccount = Objects.equals(modelAccount, authStatusManager.getUserName()); + if (currentAccount) { + allModels.putAll(copilotModels); + } // TODO: need to remove this logic after group policy is available FeatureFlags flags = CopilotCore.getPlugin().getFeatureFlags(); - if (flags == null || flags.isByokEnabled()) { + if (currentAccount && (flags == null || flags.isByokEnabled())) { allModels.putAll(registeredByokModels); } @@ -299,6 +346,9 @@ private void updateModelsForChatMode(ChatMode chatMode) { * user preference or falling back to default. */ private void validateAndSetActiveModelForMode(Map modelsForCurrentMode) { + if (getUserPreference() == null) { + return; + } CopilotModel currentActive = getActiveModel(); String restoredModelKey = restoreActiveModel(); CopilotModel restoredModel = restoredModelKey == null ? null : modelsForCurrentMode.get(restoredModelKey); @@ -335,8 +385,11 @@ private CopilotModel selectReplacementModel(Map modelsForC private void persistModelSelection(CopilotModel model) { UserPreference preference = getUserPreference(); + if (preference == null) { + return; + } preference.setChatModel(model.getModelKey()); - CompletableFuture.runAsync(this::persistUserPreference); + persistUserPreferenceAsync(preference); } private String modeToScope(ChatMode mode) { @@ -355,13 +408,22 @@ private String modeToScope(ChatMode mode) { } private void onDidCopilotStatusChange(CopilotStatusResult copilotStatusResult) { + if (copilotStatusResult.isSignedIn() != authStatusManager.isSignedIn() + || (copilotStatusResult.isSignedIn() + && !Objects.equals(copilotStatusResult.getUser(), authStatusManager.getUserName()))) { + return; + } String status = copilotStatusResult.getStatus(); switch (status) { case CopilotStatusResult.OK, CopilotStatusResult.NOT_AUTHORIZED: initializeModels(); break; default: - disposeAllSideEffects(); + ensureRealm(() -> { + modelGeneration++; + activeModelObservable.setValue(null); + modelObservable.setValue(Map.of()); + }); break; } } @@ -372,6 +434,10 @@ private void onDidCopilotStatusChange(CopilotStatusResult copilotStatusResult) { * @param modelName the name of the model */ public void setActiveModel(String modelName) { + if (getUserPreference() == null) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot change model before preferences are ready")); + return; + } Map currentModels = modelObservable.getValue(); final CopilotModel model = currentModels.values().stream() @@ -383,10 +449,6 @@ public void setActiveModel(String modelName) { if (activeModel != null && activeModel.getModelKey().equals(model.getModelKey())) { return; } - // Persist asynchronously to avoid deadlock: persistUserPreference() calls - // persistence().get() which blocks waiting for the LSP listener thread. - // If called on the UI thread while the listener is in syncExec, both threads - // deadlock. persistModelSelection(model); // Update observable @@ -400,7 +462,7 @@ public void setActiveModel(String modelName) { * @return the active model */ public CopilotModel getActiveModel() { - return activeModelObservable.getValue(); + return getUserPreference() == null ? null : activeModelObservable.getValue(); } /** @@ -489,10 +551,12 @@ public void setSelectedReasoningEffort(CopilotModel model, String reasoningEffor String key = model.getModelKey(); UserPreference preference = getUserPreference(); if (preference == null) { + CopilotCore.LOGGER.error( + new IllegalStateException("Cannot change reasoning effort before preferences are ready")); return; } preference.setReasoningEffort(key, reasoningEffort); - CompletableFuture.runAsync(this::persistUserPreference); + persistUserPreferenceAsync(preference); // Publish a fresh snapshot to drive bound picker re-renders. The actual rendering reads // resolveEffectiveReasoningEffort (which queries UserPreference), so this observable serves // purely as a change signal. @@ -509,6 +573,9 @@ public void setSelectedReasoningEffort(CopilotModel model, String reasoningEffor * failed) so a transient outage cannot wipe every stored selection. */ private void reconcileReasoningEfforts() { + if (!Objects.equals(modelAccount, authStatusManager.getUserName())) { + return; + } if (copilotModels.isEmpty() && registeredByokModels.isEmpty()) { return; } @@ -542,7 +609,7 @@ private void reconcileReasoningEfforts() { } } if (preference.setReasoningEfforts(reconciled)) { - CompletableFuture.runAsync(this::persistUserPreference); + persistUserPreferenceAsync(preference); ensureRealm(() -> reasoningEffortObservable.setValue(preference.getReasoningEffortSnapshot())); } } @@ -619,12 +686,13 @@ public void setSelectedContextWindow(CopilotModel model, Integer contextWindow) } UserPreference preference = getUserPreference(); if (preference == null) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot change context window before preferences are ready")); return; } if (!preference.setContextWindow(model.getModelKey(), contextWindow)) { return; } - CompletableFuture.runAsync(this::persistUserPreference); + persistUserPreferenceAsync(preference); // Publish a fresh snapshot to drive bound picker re-renders. The actual rendering reads // resolveEffectiveContextWindowText (which queries UserPreference), so this observable serves purely as a change // signal. @@ -641,6 +709,9 @@ public void setSelectedContextWindow(CopilotModel model, Integer contextWindow) * failed) so a transient outage cannot wipe every stored selection. */ private void reconcileContextWindows() { + if (!Objects.equals(modelAccount, authStatusManager.getUserName())) { + return; + } if (copilotModels.isEmpty() && registeredByokModels.isEmpty()) { return; } @@ -670,7 +741,7 @@ private void reconcileContextWindows() { } } if (preference.setContextWindows(reconciled)) { - CompletableFuture.runAsync(this::persistUserPreference); + persistUserPreferenceAsync(preference); ensureRealm(() -> contextWindowObservable.setValue(preference.getContextWindowSnapshot())); } } @@ -716,7 +787,16 @@ public void bindModelPicker(final DropdownButton picker) { }); // Store the side effects for later disposal - modelButtonSideEffects.put(picker, new ISideEffect[] { modelsSideEffect, activeModelSideEffect }); + ISideEffect readinessEffect = ISideEffect.create(() -> { + return preferenceStorage.getReadiness().getValue() == PreferenceStorage.State.READY + && !modelObservable.getValue().isEmpty(); + }, ready -> { + if (!picker.isDisposed()) { + picker.setEnabled(ready); + } + }); + modelButtonSideEffects.put(picker, + new ISideEffect[] { modelsSideEffect, activeModelSideEffect, readinessEffect }); // Add a dispose listener to auto-unbind when the combo is disposed picker.addDisposeListener(e -> unbindModelPicker(picker)); @@ -792,24 +872,31 @@ public void unbindActionBarForSupportVisionChange(ActionBar actionBar) { } } - private void disposeAllSideEffects() { - ensureRealm(() -> { - for (ISideEffect[] effects : modelButtonSideEffects.values()) { - for (ISideEffect effect : effects) { - if (effect != null) { - effect.dispose(); - } - } - } - }); - - modelButtonSideEffects.clear(); - } - /** * Dispose the service. */ public void dispose() { + if (disposed) { + return; + } + disposed = true; + if (modelJob != null) { + modelJob.cancel(); + } + super.ensureRealm(() -> { + readinessSideEffect.dispose(); + for (DropdownButton picker : List.copyOf(modelButtonSideEffects.keySet())) { + unbindModelPicker(picker); + } + if (actionBarSideEffect != null) { + actionBarSideEffect.dispose(); + actionBarSideEffect = null; + } + modelObservable.dispose(); + activeModelObservable.dispose(); + reasoningEffortObservable.dispose(); + contextWindowObservable.dispose(); + }); if (eventBroker != null) { eventBroker.unsubscribe(authStatusChangedEventHandler); eventBroker.unsubscribe(chatModeChangedEventHandler); @@ -819,8 +906,29 @@ public void dispose() { eventBroker.unsubscribe(customModeModelChangedEventHandler); eventBroker = null; } + } + + private UserPreference getUserPreference() { + return disposed ? null : preferenceStorage.getReadyPreferences(); + } + + private void persistUserPreferenceAsync(UserPreference preference) { + CompletableFuture.runAsync(() -> { + if (!disposed) { + preferenceStorage.persist(preference); + } + }); + } - // Should not call disposeAllSideEffects here due to issue #1301 - modelButtonSideEffects.clear(); + @Override + protected void ensureRealm(Runnable runnable) { + if (disposed) { + return; + } + super.ensureRealm(() -> { + if (!disposed) { + runnable.run(); + } + }); } } diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java new file mode 100644 index 000000000..796841f16 --- /dev/null +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java @@ -0,0 +1,489 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +package com.microsoft.copilot.eclipse.ui.chat.services; + +import java.io.IOException; +import java.io.StringReader; +import java.nio.file.Files; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; +import java.util.Objects; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.FutureTask; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; +import java.util.function.LongSupplier; + +import com.google.gson.Gson; +import com.google.gson.JsonSyntaxException; +import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; +import org.apache.commons.lang3.StringUtils; +import org.eclipse.core.databinding.observable.Realm; +import org.eclipse.core.databinding.observable.value.IObservableValue; +import org.eclipse.core.databinding.observable.value.WritableValue; +import org.eclipse.jface.databinding.swt.DisplayRealm; +import org.eclipse.swt.widgets.Display; + +import com.microsoft.copilot.eclipse.core.AuthStatusManager; +import com.microsoft.copilot.eclipse.core.CopilotAuthStatusListener; +import com.microsoft.copilot.eclipse.core.CopilotCore; +import com.microsoft.copilot.eclipse.core.chat.UserPreference; +import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; + +/** + * Owns account-scoped chat preferences and their asynchronous initial restoration. + */ +public class PreferenceStorage { + private static final Gson GSON = new Gson(); + private static final long LOAD_TIMEOUT_NANOS = TimeUnit.SECONDS.toNanos(15); + + /** + * Availability of preferences for the current account. + */ + public enum State { + UNAVAILABLE, LOADING, READY, FAILED, DISPOSED + } + + private final Object lock = new Object(); + private final Object saveLock = new Object(); + private final CopilotLanguageServerConnection connection; + private final AuthStatusManager auth; + private final ExecutorService worker; + private final ScheduledExecutorService timer; + private final LongSupplier clock; + private final FileAccess files; + private final WritableValue readiness; + private final CopilotAuthStatusListener authListener; + private State state = State.UNAVAILABLE; + private String account; + private long generation; + private Attempt attempt; + private UserPreference preferences; + private Path preferencePath; + + /** + * Creates storage without starting RPC or file work. + * + * @param connection the existing language server connection + * @param auth the existing account manager + */ + public PreferenceStorage(CopilotLanguageServerConnection connection, AuthStatusManager auth) { + this(connection, auth, DisplayRealm.getRealm(Display.getDefault()), + Executors.newCachedThreadPool(runnable -> daemonThread(runnable, "Copilot preference loading")), + Executors.newSingleThreadScheduledExecutor(runnable -> daemonThread(runnable, "Copilot preference deadline")), + System::nanoTime, new FileAccess() { + @Override + public String read(Path path) throws IOException { + return Files.readString(path); + } + + @Override + public void write(Path path, String content) throws IOException { + if (Files.notExists(path)) { + Files.createDirectories(path.getParent()); + } + Files.writeString(path, content); + } + }); + } + + PreferenceStorage(CopilotLanguageServerConnection connection, AuthStatusManager auth, Realm realm, + ExecutorService worker, ScheduledExecutorService timer, LongSupplier clock, FileAccess files) { + this.connection = connection; + this.auth = auth; + this.worker = worker; + this.timer = timer; + this.clock = clock; + this.files = files; + this.readiness = new WritableValue<>(realm, State.UNAVAILABLE, State.class); + this.account = currentAccount(); + this.authListener = status -> refreshAccount(); + auth.addCopilotAuthStatusListener(authListener); + } + + /** + * Returns current readiness without waiting for RPC, file work, or the UI thread. + * + * @return readiness applicable to the current account + */ + public State getState() { + refreshAccount(); + synchronized (lock) { + return state == State.DISPOSED || Objects.equals(account, currentAccount()) ? state : State.UNAVAILABLE; + } + } + + /** + * Returns the UI-Realm observable, available before initialization starts. + * + * @return the readiness observable + */ + public IObservableValue getReadiness() { + return readiness; + } + + /** + * Starts the first load, coalescing callers without implicitly retrying failures. + */ + public void initialize() { + start(false); + } + + /** + * Explicitly retries unavailable or failed preferences without replacing ready choices. + */ + public void retry() { + start(true); + } + + /** + * Returns the authoritative preference object only when ready for the current account. + * + * @return loaded preferences, or {@code null} when unavailable + */ + public UserPreference getReadyPreferences() { + refreshAccount(); + synchronized (lock) { + return state == State.READY && Objects.equals(account, currentAccount()) ? preferences : null; + } + } + + /** + * Synchronously saves ready preferences to their already resolved account path, without RPC. + */ + public void persist() { + persist(getReadyPreferences()); + } + + /** + * Saves a captured preference only while it remains authoritative for the current account. + */ + void persist(UserPreference expected) { + if (expected == null || getReadyPreferences() != expected) { + return; + } + synchronized (saveLock) { + refreshAccount(); + try { + Path path; + String json; + synchronized (lock) { + if (state != State.READY || preferences != expected + || !Objects.equals(account, currentAccount())) { + return; + } + path = preferencePath; + synchronized (expected) { + json = GSON.toJson(expected); + } + } + files.write(path, json); + } catch (IOException | RuntimeException exception) { + CopilotCore.LOGGER.error("Failed to save chat preferences", exception); + } + } + } + + /** + * Invalidates pending work and detaches listeners without waiting for background operations. + */ + public void dispose() { + Attempt previous; + synchronized (lock) { + if (state == State.DISPOSED) { + return; + } + previous = clear(State.DISPOSED); + } + auth.removeCopilotAuthStatusListener(authListener); + cancel(previous); + worker.shutdownNow(); + timer.shutdownNow(); + readiness.getRealm().asyncExec(() -> { + if (!readiness.isDisposed()) { + readiness.setValue(State.DISPOSED); + readiness.dispose(); + } + }); + } + + private static Thread daemonThread(Runnable runnable, String name) { + Thread thread = new Thread(runnable, name); + thread.setDaemon(true); + return thread; + } + + private String currentAccount() { + String user = auth.getUserName(); + return auth.isSignedIn() && StringUtils.isNotBlank(user) ? user : null; + } + + private void refreshAccount() { + Attempt previous; + long version; + synchronized (lock) { + String current = currentAccount(); + if (state == State.DISPOSED || Objects.equals(account, current)) { + return; + } + account = current; + previous = clear(State.UNAVAILABLE); + version = generation; + } + cancel(previous); + publishState(version, State.UNAVAILABLE); + } + + private Attempt clear(State next) { + final Attempt previous = attempt; + attempt = null; + preferences = null; + preferencePath = null; + state = next; + generation++; + return previous; + } + + private void start(boolean retry) { + refreshAccount(); + Attempt next; + synchronized (lock) { + if (account == null || state == State.DISPOSED || state == State.LOADING || state == State.READY + || (!retry && state == State.FAILED)) { + return; + } + state = State.LOADING; + next = new Attempt(++generation, account, clock.getAsLong()); + attempt = next; + } + publishState(next.generation, State.LOADING); + try { + ScheduledFuture timeout = timer.schedule(() -> fail(next, null), 15, TimeUnit.SECONDS); + synchronized (lock) { + if (attempt == next) { + next.timeout = timeout; + } else { + timeout.cancel(false); + } + } + dispatch(next, () -> resolve(next)); + } catch (RuntimeException exception) { + fail(next, exception); + } + } + + private void dispatch(Attempt pending, Runnable runnable) { + FutureTask task = new FutureTask<>(() -> { + if (isCurrent(pending)) { + runnable.run(); + } + }, null); + synchronized (lock) { + if (!matches(pending)) { + return; + } + pending.work = task; + } + try { + worker.execute(task); + } catch (RuntimeException exception) { + fail(pending, exception); + } + } + + private void resolve(Attempt pending) { + try { + CompletableFuture rpc = connection.persistence(); + if (rpc == null) { + throw new IllegalStateException("Persistence request returned no future"); + } + boolean applicable; + synchronized (lock) { + applicable = matches(pending); + if (applicable) { + pending.rpc = rpc; + } + } + if (!applicable) { + rpc.cancel(true); + return; + } + rpc.whenComplete((result, failure) -> { + if (failure != null) { + fail(pending, failure); + } else { + dispatch(pending, () -> read(pending, result)); + } + }); + } catch (RuntimeException exception) { + fail(pending, exception); + } + } + + private void read(Attempt pending, ChatPersistence result) { + try { + if (result == null || StringUtils.isBlank(result.getPath())) { + throw new IllegalArgumentException("Persistence response has no path"); + } + Path directory = Path.of(result.getPath()); + if (!directory.isAbsolute()) { + throw new IllegalArgumentException("Persistence path is not absolute"); + } + Path path = directory.resolve(pending.account).resolve("pref.json"); + UserPreference restored; + try { + restored = parse(files.read(path)); + } catch (NoSuchFileException exception) { + restored = new UserPreference(); + } + publishPreferences(pending, path, restored); + } catch (IOException | RuntimeException exception) { + fail(pending, exception); + } + } + + private UserPreference parse(String json) throws IOException { + try (JsonReader reader = new JsonReader(new StringReader(json))) { + configureStrictJson(reader); + if (reader.peek() != JsonToken.BEGIN_OBJECT) { + throw new JsonSyntaxException("Preferences must contain an object"); + } + // The adapter preserves JsonReader strictness, unlike Gson.fromJson(JsonReader, Class). + UserPreference restored = GSON.getAdapter(UserPreference.class).read(reader); + if (reader.peek() != JsonToken.END_DOCUMENT) { + throw new JsonSyntaxException("Unexpected content after preferences"); + } + restored.setReasoningEfforts(restored.getReasoningEffortSnapshot()); + restored.setContextWindows(restored.getContextWindowSnapshot()); + return restored; + } + } + + private void configureStrictJson(JsonReader reader) throws IOException { + try { + // Older supported Eclipse targets bundle Gson 2.10, before the Strictness API. + Class strictness = Class.forName("com.google.gson.Strictness"); + JsonReader.class.getMethod("setStrictness", strictness).invoke(reader, strictness.getField("STRICT").get(null)); + } catch (ClassNotFoundException | NoSuchMethodException exception) { + reader.setLenient(false); + } catch (ReflectiveOperationException exception) { + throw new IOException("Cannot configure strict preference JSON parsing", exception); + } + } + + private void publishPreferences(Attempt pending, Path path, UserPreference restored) { + readiness.getRealm().asyncExec(() -> { + if (!isCurrent(pending)) { + return; + } + synchronized (lock) { + if (!matches(pending)) { + return; + } + preferences = restored; + preferencePath = path; + state = State.READY; + attempt = null; + if (!readiness.isDisposed()) { + readiness.setValue(State.READY); + } + } + if (pending.timeout != null) { + pending.timeout.cancel(false); + } + }); + } + + private boolean matches(Attempt pending) { + return state == State.LOADING && attempt == pending && generation == pending.generation + && Objects.equals(account, pending.account) && Objects.equals(account, currentAccount()); + } + + private boolean isCurrent(Attempt pending) { + refreshAccount(); + synchronized (lock) { + if (!matches(pending)) { + return false; + } + if (clock.getAsLong() - pending.started < LOAD_TIMEOUT_NANOS) { + return true; + } + } + fail(pending, null); + return false; + } + + private void fail(Attempt pending, Throwable failure) { + refreshAccount(); + long version; + synchronized (lock) { + if (!matches(pending)) { + return; + } + clear(State.FAILED); + version = generation; + } + cancel(pending); + if (failure != null) { + CopilotCore.LOGGER.error("Failed to load chat preferences", failure); + } + publishState(version, State.FAILED); + } + + private void publishState(long version, State next) { + readiness.getRealm().asyncExec(() -> { + refreshAccount(); + synchronized (lock) { + if (generation != version || state != next || readiness.isDisposed()) { + return; + } + readiness.setValue(next); + } + }); + } + + private void cancel(Attempt pending) { + if (pending == null) { + return; + } + if (pending.timeout != null) { + pending.timeout.cancel(false); + } + if (pending.work != null) { + pending.work.cancel(false); + } + if (pending.rpc != null) { + pending.rpc.cancel(true); + } + } + + interface FileAccess { + String read(Path path) throws IOException; + + void write(Path path, String content) throws IOException; + } + + /** + * Account and lifecycle identity captured before dispatching any load work. + */ + private static class Attempt { + private final long generation; + private final String account; + private final long started; + private volatile CompletableFuture rpc; + private volatile FutureTask work; + private volatile ScheduledFuture timeout; + + Attempt(long generation, String account, long started) { + this.generation = generation; + this.account = account; + this.started = started; + } + } +} 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 0ca4b9699..cc6a793de 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; @@ -20,7 +21,6 @@ import org.osgi.service.event.EventHandler; import com.microsoft.copilot.eclipse.core.AuthStatusManager; -import com.microsoft.copilot.eclipse.core.CopilotAuthStatusListener; import com.microsoft.copilot.eclipse.core.CopilotCore; import com.microsoft.copilot.eclipse.core.FeatureFlags; import com.microsoft.copilot.eclipse.core.chat.BuiltInChatMode; @@ -46,7 +46,10 @@ /** * Service for managing chat modes and input navigation. */ -public class UserPreferenceService extends ChatBaseService implements CopilotAuthStatusListener { +public class UserPreferenceService extends ChatBaseService { + private final PreferenceStorage preferenceStorage; + private ISideEffect readinessSideEffect; + private volatile boolean disposed; 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 @@ -64,31 +67,37 @@ public class UserPreferenceService extends ChatBaseService implements CopilotAut /** * Constructor for the UserPreferenceService. */ - public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager) { + public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthStatusManager authStatusManager, + PreferenceStorage preferenceStorage) { super(lsConnection, authStatusManager); + this.preferenceStorage = preferenceStorage; - this.authStatusManager.addCopilotAuthStatusListener(this); ensureRealm(() -> { chatModeObservable = new WritableValue<>(getAvailableChatModes(), String[].class); activeChatModeObservable = new WritableValue<>(null, ChatMode.class); activeModeNameOrIdObservable = new WritableValue<>(null, String.class); + readinessSideEffect = ISideEffect.create(preferenceStorage.getReadiness()::getValue, state -> { + if (state == PreferenceStorage.State.READY) { + init(); + } else { + activeChatModeObservable.setValue(null); + activeModeNameOrIdObservable.setValue(null); + inputNavigation = new InputNavigation(); + } + }); }); initializeEventHandlers(); subscribeToEvents(); - init(); + preferenceStorage.initialize(); } private void initializeEventHandlers() { authStatusChangedEventHandler = event -> { Object property = event.getProperty(IEventBroker.DATA); if (property instanceof CopilotStatusResult statusResult) { - // 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()) { - clearUserPreferenceCache(); - this.inputNavigation = null; - } else { + if (statusResult.isSignedIn() && authStatusManager.isSignedIn() + && Objects.equals(statusResult.getUser(), authStatusManager.getUserName())) { // User has signed in - reload built-in modes to ensure we have the latest modes for this user try { BuiltInChatModeManager.INSTANCE.reloadModes(); @@ -99,9 +108,6 @@ private void initializeEventHandlers() { 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); } @@ -136,7 +142,7 @@ private void subscribeToEvents() { } private void init() { - if (authStatusManager.isSignedIn()) { + if (preferenceStorage.getState() == PreferenceStorage.State.READY) { // Initialize chat mode preferences String chatModeName = restoreChatModeName(); @@ -150,11 +156,14 @@ private void init() { } final ChatMode viewMode = rawViewMode; + inputNavigation = new InputNavigation(restoreUserInputs()); ensureRealm(() -> { - activeChatModeObservable.setValue(viewMode); activeModeNameOrIdObservable.setValue(chatModeName); + activeChatModeObservable.setValue(viewMode); }); - inputNavigation = new InputNavigation(restoreUserInputs()); + if (eventBroker != null) { + eventBroker.post(CopilotEventConstants.TOPIC_CHAT_MODE_CHANGED, viewMode); + } } } @@ -201,19 +210,6 @@ private List restoreUserInputs() { return new ArrayList<>(); } - @Override - public void onDidCopilotStatusChange(CopilotStatusResult copilotStatusResult) { - String status = copilotStatusResult.getStatus(); - switch (status) { - case CopilotStatusResult.OK, CopilotStatusResult.NOT_AUTHORIZED: - init(); - break; - default: - disposeAllSideEffects(); - break; - } - } - /** * Get available chat modes based on feature flags. Includes built-in modes, custom modes, and "Add New Mode" option * with separators. @@ -249,6 +245,11 @@ private String[] getAvailableChatModes() { * @param chatModeNameOrId the name/ID of the chat mode to set */ public void setActiveChatMode(String chatModeNameOrId) { + UserPreference preference = getUserPreference(); + if (preference == null) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot change chat mode before preferences are ready")); + return; + } if (StringUtils.isBlank(chatModeNameOrId)) { return; } @@ -274,7 +275,6 @@ public void setActiveChatMode(String chatModeNameOrId) { ChatMode uiViewMode = getViewModeForModeName(chatModeNameOrId); // Step 4: Persist user preference - UserPreference preference = getUserPreference(); preference.setChatModeName(chatModeNameOrId); persistUserPreference(); @@ -302,6 +302,9 @@ public void setActiveChatMode(String chatModeNameOrId) { * @return the active chat mode for UI rendering */ public ChatMode getActiveChatMode() { + if (getUserPreference() == null) { + return ChatMode.Agent; + } ChatMode activeChatMode = activeChatModeObservable.getValue(); if (activeChatMode != null) { return activeChatMode; @@ -319,7 +322,7 @@ public ChatMode getActiveChatMode() { * @return the active mode name (for built-in modes) or custom mode ID (for custom modes) */ public String getActiveModeNameOrId() { - return activeModeNameOrIdObservable.getValue(); + return getUserPreference() == null ? null : activeModeNameOrIdObservable.getValue(); } /** @@ -407,7 +410,13 @@ public void bindChatModePicker(final DropdownButton picker) { } }); - chatModeButtonSideEffects.put(picker, new ISideEffect[] { groupsSideEffect, selectionSideEffect }); + ISideEffect readinessEffect = ISideEffect.create(preferenceStorage.getReadiness()::getValue, state -> { + if (!picker.isDisposed()) { + picker.setEnabled(state == PreferenceStorage.State.READY); + } + }); + chatModeButtonSideEffects.put(picker, + new ISideEffect[] { groupsSideEffect, selectionSideEffect, readinessEffect }); // Add a dispose listener to auto-unbind when the dropdown button is disposed picker.addDisposeListener(e -> unbindChatModePicker(picker)); }); @@ -513,8 +522,12 @@ public void unbindChatView() { * Add input to the input history. */ public void addInputToHistory(String input) { - inputNavigation.add(input); UserPreference preference = getUserPreference(); + if (preference == null) { + CopilotCore.LOGGER.error(new IllegalStateException("Cannot record chat input before preferences are ready")); + return; + } + inputNavigation.add(input); preference.setUserInputs(inputNavigation.getInputHistoryList()); } @@ -525,6 +538,9 @@ public void addInputToHistory(String input) { * @return the previous input or an empty string if at the top of the history. */ public String getPreviousInput(String currentInput) { + if (getUserPreference() == null) { + return StringUtils.EMPTY; + } if (inputNavigation.atBottom() && StringUtils.isNotEmpty(currentInput)) { inputNavigation.add(currentInput); inputNavigation.updateCursorPosition(inputNavigation.size() - 1); @@ -532,7 +548,15 @@ public String getPreviousInput(String currentInput) { return inputNavigation.navigateUp(); } + /** + * Returns the next input, or an empty string when preference history is unavailable. + * + * @return next input in the history + */ public String getNextInput() { + if (getUserPreference() == null) { + return StringUtils.EMPTY; + } return inputNavigation.navigateDown(); } @@ -564,7 +588,12 @@ public boolean isSkipGitHubJobConfirmDialog() { */ public void setSkipGitHubJobConfirmDialog(boolean skip) { UserPreference preference = getUserPreference(); - if (preference != null && preference.isSkipGitHubJobConfirmDialog() != skip) { + if (preference == null) { + CopilotCore.LOGGER.error( + new IllegalStateException("Cannot change confirmation preferences before preferences are ready")); + return; + } + if (preference.isSkipGitHubJobConfirmDialog() != skip) { preference.setSkipGitHubJobConfirmDialog(skip); persistUserPreference(); } @@ -574,11 +603,21 @@ public void setSkipGitHubJobConfirmDialog(boolean skip) { * Dispose of the service. */ public void dispose() { + if (disposed) { + return; + } persistUserPreference(); - // Ideally we should dispose all side effects and observable here. But since the service is - // singleton and will only be disposed when the bundle is stopped. So right now they are not - // explicitly disposed here. - this.authStatusManager.removeCopilotAuthStatusListener(this); + disposed = true; + super.ensureRealm(() -> { + readinessSideEffect.dispose(); + unbindChatView(); + for (DropdownButton picker : List.copyOf(chatModeButtonSideEffects.keySet())) { + unbindChatModePicker(picker); + } + chatModeObservable.dispose(); + activeChatModeObservable.dispose(); + activeModeNameOrIdObservable.dispose(); + }); if (eventBroker != null) { eventBroker.unsubscribe(authStatusChangedEventHandler); @@ -603,18 +642,28 @@ private boolean isModeAvailable(String modeNameOrId) { return false; } - private void disposeAllSideEffects() { - ensureRealm(() -> { - // Dispose chat mode combo side effects - for (ISideEffect[] effects : chatModeButtonSideEffects.values()) { - for (ISideEffect effect : effects) { - if (effect != null) { - effect.dispose(); - } - } + private UserPreference getUserPreference() { + return disposed ? null : preferenceStorage.getReadyPreferences(); + } + + /** + * Saves the currently loaded preferences without resolving another persistence path. + */ + public void persistUserPreference() { + if (!disposed) { + preferenceStorage.persist(); + } + } + + @Override + protected void ensureRealm(Runnable runnable) { + if (disposed) { + return; + } + super.ensureRealm(() -> { + if (!disposed) { + runnable.run(); } }); - - chatModeButtonSideEffects.clear(); } } From 450bb7814e20b8e8143ecd4c8aa6d40a05129d2a Mon Sep 17 00:00:00 2001 From: Sheng Chen Date: Sun, 20 Sep 2026 16:02:07 +0800 Subject: [PATCH 2/4] Close asynchronous preference initialization and recovery gaps Keep cold built-in mode discovery off SWT, preserve selected-mode identity until discovery completes, and expose independent manual recovery without reloading ready preferences. Restore model vision observation, guard disposed access, tolerate unavailable image models, and reject legacy Gson JSON extensions. Harden UI regressions so SWT failures reach JUnit. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../core/chat/BuiltInChatModeManager.java | 33 +- .../chat/service/BuiltInChatModeService.java | 16 +- .../eclipse/ui/chat/ReferencedFileTest.java | 86 ++++- .../ui/chat/services/ModelServiceTests.java | 64 ++++ .../chat/services/PreferenceStorageTest.java | 30 +- .../services/UserPreferenceServiceTest.java | 331 +++++++++++++++++- .../copilot/eclipse/ui/chat/ActionBar.java | 12 +- .../copilot/eclipse/ui/chat/ChatView.java | 3 +- .../copilot/eclipse/ui/chat/Messages.java | 2 + .../eclipse/ui/chat/PreferenceStatus.java | 45 ++- .../eclipse/ui/chat/ReferencedFile.java | 8 +- .../eclipse/ui/chat/messages.properties | 2 + .../ui/chat/services/ModelService.java | 6 +- .../ui/chat/services/PreferenceStorage.java | 42 ++- .../chat/services/UserPreferenceService.java | 122 ++++++- .../copilot/eclipse/ui/i18n/Messages.java | 1 + .../eclipse/ui/i18n/messages.properties | 1 + 17 files changed, 696 insertions(+), 108 deletions(-) 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 079628bf1..fd4199bbd 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,34 +5,14 @@ import java.util.ArrayList; import java.util.List; -import java.util.concurrent.CopyOnWriteArrayList; - -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. + * Shared snapshot of built-in chat modes discovered asynchronously by the chat lifecycle. */ public enum BuiltInChatModeManager { INSTANCE; - private final BuiltInChatModeService service; - private List builtInModes; - - BuiltInChatModeManager() { - this.service = new BuiltInChatModeService(); - this.builtInModes = new CopyOnWriteArrayList<>(); - loadModesSync(); - } - - 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<>(); - } - } + private volatile List builtInModes = List.of(); public List getBuiltInModes() { return new ArrayList<>(builtInModes); @@ -60,10 +40,11 @@ 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. + * Publishes a completed discovery after its owner has validated the account and lifecycle. + * + * @param modes the modes applicable to the current account */ - public void reloadModes() { - loadModesSync(); + public void updateModes(List modes) { + builtInModes = List.copyOf(modes); } } \ No newline at end of file diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java index cc3765064..e005f198c 100644 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java +++ b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java @@ -30,12 +30,21 @@ public class BuiltInChatModeService { * don't depend on workspace context, the LSP API enforces this parameter. */ public CompletableFuture> loadBuiltInModes() { - ConversationModesParams params = new ConversationModesParams(Collections.emptyList()); + return loadBuiltInModes(CopilotCore.getPlugin().getCopilotLanguageServer()); + } - CopilotLanguageServerConnection lspConnection = CopilotCore.getPlugin().getCopilotLanguageServer(); + /** + * Loads built-in modes using the owning chat lifecycle's language-server connection. + * + * @param lspConnection the connection used by the chat services + * @return the discovered built-in modes + */ + public CompletableFuture> loadBuiltInModes( + CopilotLanguageServerConnection lspConnection) { if (lspConnection == null) { return CompletableFuture.completedFuture(new ArrayList<>()); } + ConversationModesParams params = new ConversationModesParams(Collections.emptyList()); return lspConnection.listConversationModes(params).thenApply(conversationModes -> { List builtInModes = new ArrayList<>(); @@ -58,9 +67,6 @@ public CompletableFuture> loadBuiltInModes() { } return builtInModes; - }).exceptionally(ex -> { - CopilotCore.LOGGER.error("Failed to load built-in modes", ex); - return new ArrayList<>(); }); } diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFileTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFileTest.java index 75a925fb2..11dd285ac 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFileTest.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFileTest.java @@ -3,9 +3,13 @@ package com.microsoft.copilot.eclipse.ui.chat; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +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.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.spy; @@ -15,8 +19,11 @@ import java.lang.reflect.Method; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.core.resources.IFile; +import org.eclipse.jface.preference.PreferenceStore; +import org.eclipse.jface.resource.ImageRegistry; import org.eclipse.swt.SWT; import org.eclipse.swt.widgets.Composite; import org.eclipse.swt.widgets.Control; @@ -30,18 +37,21 @@ import org.mockito.MockedStatic; import org.mockito.junit.jupiter.MockitoExtension; +import com.microsoft.copilot.eclipse.core.AuthStatusManager; +import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatMode; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotModel; import com.microsoft.copilot.eclipse.ui.CopilotUi; +import com.microsoft.copilot.eclipse.ui.chat.contextwindow.ContextWindowService; import com.microsoft.copilot.eclipse.ui.chat.services.AgentToolService; import com.microsoft.copilot.eclipse.ui.chat.services.ChatFontService; import com.microsoft.copilot.eclipse.ui.chat.services.ChatServiceManager; import com.microsoft.copilot.eclipse.ui.chat.services.McpConfigService; import com.microsoft.copilot.eclipse.ui.chat.services.ModelService; +import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage; import com.microsoft.copilot.eclipse.ui.chat.services.ReferencedFileService; import com.microsoft.copilot.eclipse.ui.chat.services.UserPreferenceService; import com.microsoft.copilot.eclipse.ui.chat.tools.JavaDebuggerToolAdapter; -import com.microsoft.copilot.eclipse.ui.utils.SwtUtils; @ExtendWith(MockitoExtension.class) class ReferencedFileTest { @@ -68,14 +78,21 @@ class ReferencedFileTest { private AgentToolService mockAgentToolService; @Mock private ChatFontService mockChatFontService; + @Mock + private ContextWindowService mockContextWindowService; + @Mock + private AuthStatusManager mockAuthStatusManager; + @Mock + private CopilotLanguageServerConnection mockConnection; private Shell shell; private ActionBar actionBar; + private PreferenceStorage preferenceStorage; private MockedStatic mockedCopilotUi; @BeforeEach void setUp() { - SwtUtils.invokeOnDisplayThread(() -> { + runOnUi(() -> { setupSwtComponents(); setupMockFiles(); setupMockServices(); @@ -84,16 +101,22 @@ void setUp() { @AfterEach void tearDown() { - SwtUtils.invokeOnDisplayThread(() -> { - if (actionBar != null && !actionBar.isDisposed()) { - actionBar.dispose(); - } - if (shell != null && !shell.isDisposed()) { - shell.dispose(); + try { + runOnUi(() -> { + if (actionBar != null && !actionBar.isDisposed()) { + actionBar.dispose(); + } + if (shell != null && !shell.isDisposed()) { + shell.dispose(); + } + if (preferenceStorage != null) { + preferenceStorage.dispose(); + } + }); + } finally { + if (mockedCopilotUi != null) { + mockedCopilotUi.close(); } - }); - if (mockedCopilotUi != null) { - mockedCopilotUi.close(); } } @@ -110,8 +133,13 @@ private void setupMockFiles() { } private void setupMockServices() { + ImageRegistry imageRegistry = CopilotUi.getPlugin().getImageRegistry(); mockedCopilotUi = mockStatic(CopilotUi.class); mockedCopilotUi.when(CopilotUi::getPlugin).thenReturn(mockCopilotUi); + when(mockCopilotUi.getImageRegistry()).thenReturn(imageRegistry); + when(mockCopilotUi.getPreferenceStore()).thenReturn(new PreferenceStore()); + preferenceStorage = new PreferenceStorage(mockConnection, mockAuthStatusManager); + preferenceStorage.initialize(); lenient().when(mockModel.getModelName()).thenReturn("test-model"); lenient().when(mockModelService.getActiveModel()).thenReturn(mockModel); @@ -121,18 +149,38 @@ private void setupMockServices() { lenient().when(mockChatServiceManager.getMcpConfigService()).thenReturn(mockMcpConfigService); lenient().when(mockChatServiceManager.getAgentToolService()).thenReturn(mockAgentToolService); lenient().when(mockChatServiceManager.getChatFontService()).thenReturn(mockChatFontService); + when(mockChatServiceManager.getContextWindowService()).thenReturn(mockContextWindowService); + when(mockChatServiceManager.getPreferenceStorage()).thenReturn(preferenceStorage); lenient().when(mockAgentToolService.getTool(JavaDebuggerToolAdapter.TOOL_NAME)).thenReturn(null); lenient().when(mockUserPreferenceService.getActiveChatMode()).thenReturn(ChatMode.Ask); lenient().when(mockCopilotUi.getChatServiceManager()).thenReturn(mockChatServiceManager); actionBar = spy(new ActionBar(shell, SWT.NONE, mockChatServiceManager)); } + @Test + void testImageReference_ModelUnavailable_ShowsUnsupportedChipAndLocalizedTooltip() { + runOnUi(() -> { + when(mockModelService.getActiveModel()).thenReturn(null); + assertEquals(PreferenceStorage.State.UNAVAILABLE, preferenceStorage.getState()); + + ReferencedFile image = assertDoesNotThrow(() -> new ReferencedFile(shell, mockImageFile, true)); + + assertSame(mockImageFile, image.getFile()); + assertTrue(image.isFileUnSupported()); + assertTrue(image.getVisible()); + assertEquals(3, image.getChildren().length); + for (Control child : image.getChildren()) { + assertEquals("Images are unavailable until a model is ready.", child.getToolTipText()); + } + }); + } + /** * Test ActionBar creates ReferencedFiles with correct strikeThrough state based on model vision support. */ @Test void testStrikeThroughBehavior() { - SwtUtils.invokeOnDisplayThread(() -> { + runOnUi(() -> { List testFiles = Arrays.asList(mockImageFile, mockTextFile); callUpdateReferencedFilesInternal(actionBar, testFiles, true); @@ -161,6 +209,20 @@ void testStrikeThroughBehavior() { }); } + private static void runOnUi(Runnable runnable) { + AtomicReference failure = new AtomicReference<>(); + Display.getDefault().syncExec(() -> { + try { + runnable.run(); + } catch (Throwable error) { + failure.set(error); + } + }); + if (failure.get() != null) { + fail("UI operation failed", failure.get()); + } + } + /** * Helper method to call private updateReferencedFilesInternal method using reflection. */ diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java index 2141f01ff..e006a1eeb 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelServiceTests.java @@ -6,6 +6,7 @@ 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.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; @@ -16,12 +17,16 @@ import java.nio.file.Path; import java.util.List; import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; import com.google.gson.Gson; +import org.eclipse.core.databinding.observable.Realm; +import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; import org.eclipse.e4.core.services.events.IEventBroker; +import org.eclipse.jface.databinding.swt.DisplayRealm; import org.eclipse.swt.widgets.Display; import org.eclipse.ui.PlatformUI; import org.junit.jupiter.api.AfterEach; @@ -40,6 +45,8 @@ import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotModel; +import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotModel.CopilotModelCapabilities; +import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotModel.CopilotModelCapabilitiesSupports; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotScope; import com.microsoft.copilot.eclipse.core.lsp.protocol.byok.ByokListModelResponse; @@ -91,6 +98,63 @@ void tearDown() { preferenceStorage.dispose(); } + @Test + void testInitialize_PendingPreferences_UpdatesVisionBindingWhenRestored() throws InterruptedException { + CompletableFuture pending = new CompletableFuture<>(); + when(lsConnection.persistence()).thenReturn(pending); + CopilotModel visionModel = createModel("vision", "Vision", true); + visionModel.setCapabilities(new CopilotModelCapabilities( + new CopilotModelCapabilitiesSupports(true, List.of(), false), null)); + when(lsConnection.listModels()) + .thenReturn(CompletableFuture.completedFuture(new CopilotModel[] {visionModel})); + AtomicBoolean supportsVision = new AtomicBoolean(); + AtomicBoolean uiActionProcessed = new AtomicBoolean(); + AtomicReference binding = new AtomicReference<>(); + Display.getDefault().syncExec(() -> { + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); + Realm.runWithDefault(DisplayRealm.getRealm(Display.getDefault()), + () -> binding.set(ISideEffect.create(modelService::isVisionSupported, supportsVision::set))); + Display.getDefault().asyncExec(() -> uiActionProcessed.set(true)); + }); + try { + waitUntil(uiActionProcessed::get); + assertFalse(pending.isDone()); + assertFalse(supportsVision.get()); + assertEquals(PreferenceStorage.State.LOADING, preferenceStorage.getState()); + + ChatPersistence persistence = new ChatPersistence(); + persistence.setPath(persistenceDirectory.toString()); + pending.complete(persistence); + + waitUntil(supportsVision::get); + assertEquals(PreferenceStorage.State.READY, preferenceStorage.getState()); + assertEquals("vision", getActiveModelId()); + } finally { + Display.getDefault().syncExec(() -> binding.get().dispose()); + } + } + + @Test + void testDispose_ActiveModelIsUnavailableWithoutAccessingDisposedObservable() throws Exception { + CopilotModel defaultModel = createModel("gpt-4o", "GPT-4o", true); + when(lsConnection.listModels()) + .thenReturn(CompletableFuture.completedFuture(new CopilotModel[] {defaultModel})); + modelService = new ModelService(lsConnection, authStatusManager, preferenceStorage); + waitUntil(() -> defaultModel.getId().equals(getActiveModelId())); + + CompletableFuture result = new CompletableFuture<>(); + Display.getDefault().syncExec(() -> { + try { + modelService.dispose(); + result.complete(modelService.getActiveModel()); + } catch (Throwable failure) { + result.completeExceptionally(failure); + } + }); + + assertNull(result.get(5, TimeUnit.SECONDS)); + } + @Test void testAutoModelAvailableWhenEditorPreviewDisabled() throws InterruptedException { CopilotModel defaultModel = createModel("gpt-4o", "GPT-4o", true); diff --git a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java index 52a9ab84f..de41c3e86 100644 --- a/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java +++ b/com.microsoft.copilot.eclipse.ui.test/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorageTest.java @@ -24,6 +24,7 @@ import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.util.List; import java.util.Queue; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ConcurrentLinkedQueue; @@ -37,7 +38,6 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.condition.EnabledIf; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.NullAndEmptySource; import org.junit.jupiter.params.provider.ValueSource; @@ -165,22 +165,28 @@ void testInitialize_NullModelMaps_NormalizesSafeDefaults() throws Exception { assertTrue(restored.setContextWindow("model-a", 128000)); } - @Test - @EnabledIf("supportsStrictJson") - void testInitialize_GsonSupportsStrictJson_RejectsUnescapedControlCharacters() throws Exception { - when(files.read(any(Path.class))).thenReturn("{\"chatModel\":\"line\nbreak\"}"); + @ParameterizedTest + @ValueSource(strings = {"{\"chatModel\":\"line\nbreak\"}", "{\"chatModel\":\"tab\tcharacter\"}", + "{\"chatModel\":\"escaped\\\nnewline\"}", "{\"chatModel\":\"single\\'quote\"}", + "{\"skipGitHubJobConfirmDialog\":TRUE}", "{\"chatModel\":NULL}"}) + void testInitialize_LegacyGsonExtensions_FailWithoutOverwriting(String content) throws Exception { + when(files.read(any(Path.class))).thenReturn(content); load(); assertFailedWithoutWrites(); } - private static boolean supportsStrictJson() { - try { - Class.forName("com.google.gson.Strictness"); - return true; - } catch (ClassNotFoundException exception) { - return false; - } + @Test + void testInitialize_ValidJsonEscapes_RestoresOriginalText() throws Exception { + when(files.read(any(Path.class))).thenReturn(""" + {"userInputs":["line\\nbreak","tab\\tcharacter","quote\\" and slash\\\\","\\u0041", + "single'quote"],"skipGitHubJobConfirmDialog":false} + """); + load(); + + assertEquals(State.READY, storage.getState()); + assertEquals(List.of("line\nbreak", "tab\tcharacter", "quote\" and slash\\", "A", "single'quote"), + storage.getReadyPreferences().getUserInputs()); } @Test 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 5c07a19a5..ffc8eb6eb 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 @@ -7,10 +7,14 @@ 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.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.timeout; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; @@ -24,32 +28,45 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.function.BooleanSupplier; import org.eclipse.swt.SWT; +import org.eclipse.core.databinding.observable.Realm; +import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; +import org.eclipse.e4.core.services.events.IEventBroker; import org.eclipse.jface.databinding.swt.DisplayRealm; import org.eclipse.swt.widgets.Display; import org.eclipse.swt.widgets.Shell; import org.eclipse.swt.widgets.Label; import org.eclipse.swt.widgets.Link; +import org.eclipse.ui.PlatformUI; import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; import org.junit.jupiter.api.extension.ExtendWith; import org.junit.jupiter.api.io.TempDir; import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.osgi.service.event.EventHandler; import com.microsoft.copilot.eclipse.core.AuthStatusManager; import com.microsoft.copilot.eclipse.core.CopilotAuthStatusListener; import com.microsoft.copilot.eclipse.core.CopilotCore; import com.microsoft.copilot.eclipse.core.FeatureFlags; +import com.microsoft.copilot.eclipse.core.chat.BuiltInChatModeManager; +import com.microsoft.copilot.eclipse.core.chat.UserPreference; +import com.microsoft.copilot.eclipse.core.events.CopilotEventConstants; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatMode; import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatPersistence; import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotStatusResult; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ConversationMode; import com.microsoft.copilot.eclipse.ui.swt.DropdownButton; import com.microsoft.copilot.eclipse.ui.chat.Messages; import com.microsoft.copilot.eclipse.ui.chat.PreferenceStatus; @@ -69,9 +86,16 @@ class UserPreferenceServiceTest { private DropdownButton picker; private PreferenceStatus status; + @BeforeEach + void setUp() { + lenient().when(connection.listConversationModes(any())) + .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { + builtInMode("Ask", "Ask"), builtInMode("Agent", "Agent"), builtInMode("Plan", "Plan")})); + } + @AfterEach void tearDown() { - Display.getDefault().syncExec(() -> { + runOnUi(() -> { if (shell != null) { shell.dispose(); } @@ -84,6 +108,257 @@ void tearDown() { }); } + @Test + void testFirstInitialization_PendingModeDiscovery_ProcessesSwtAndPublishesModesAfterCompletion() throws Exception { + when(auth.isSignedIn()).thenReturn(true); + when(auth.getUserName()).thenReturn("user"); + CompletableFuture preferences = new CompletableFuture<>(); + CompletableFuture modes = new CompletableFuture<>(); + when(connection.persistence()).thenReturn(preferences); + when(connection.listConversationModes(any())).thenReturn(modes); + writePreferences("{\"chatModeName\":\"Ask\"}"); + CompletableFuture uiAction = new CompletableFuture<>(); + Display.getDefault().asyncExec(() -> { + try { + storage = new PreferenceStorage(connection, auth); + createControls(); + Display.getDefault().asyncExec(() -> { + try { + uiAction.complete(!picker.getEnabled() && storage.getState() == PreferenceStorage.State.LOADING); + } catch (Throwable error) { + uiAction.completeExceptionally(error); + } + }); + } catch (Throwable error) { + uiAction.completeExceptionally(error); + } + }); + try { + assertTrue(uiAction.get(5, TimeUnit.SECONDS), "Cold mode discovery must not prevent SWT processing"); + verify(connection, timeout(5000)).listConversationModes(any()); + assertFalse(modes.isDone()); + assertFalse(preferences.isDone()); + ConversationMode ask = new ConversationMode(); + ask.setId("pending-discovery-ask"); + ask.setName("Ask"); + ask.setKind("Ask"); + ask.setBuiltIn(true); + modes.complete(new ConversationMode[] {ask}); + preferences.complete(persistence()); + awaitUi(() -> picker.getEnabled() && "Ask".equals(picker.getSelectedItemId()) + && BuiltInChatModeManager.INSTANCE.getBuiltInModeById("pending-discovery-ask") != null); + runOnUi(() -> assertEquals(ChatMode.Ask, service.getActiveChatMode())); + } finally { + modes.complete(new ConversationMode[0]); + preferences.completeExceptionally(new IllegalStateException("test cleanup")); + } + } + + @Test + void testInitialization_PreferencesBeforeModeDiscovery_GatesPlanActionsAndRefreshesConsumers() throws Exception { + CompletableFuture modes = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(modes); + writePreferences("{\"chatModeName\":\"Plan\"}"); + AtomicBoolean actionsReady = new AtomicBoolean(); + AtomicBoolean resolvedModeNotification = new AtomicBoolean(); + AtomicInteger notifications = new AtomicInteger(); + AtomicReference readinessBinding = new AtomicReference<>(); + IEventBroker broker = PlatformUI.getWorkbench().getService(IEventBroker.class); + EventHandler listener = event -> { + notifications.incrementAndGet(); + if (BuiltInChatModeManager.INSTANCE.getBuiltInModeById("delayed-plan") != null) { + resolvedModeNotification.set(event.getProperty(IEventBroker.DATA) == ChatMode.Agent); + } + }; + broker.subscribe(CopilotEventConstants.TOPIC_CHAT_MODE_CHANGED, listener); + try { + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> storage.getState() == PreferenceStorage.State.READY && notifications.get() > 0); + runOnUi(() -> { + Realm.runWithDefault(DisplayRealm.getRealm(Display.getDefault()), () -> readinessBinding.set( + ISideEffect.create(service::isActiveModeReady, actionsReady::set))); + assertEquals("Plan", service.getActiveModeNameOrId()); + assertFalse(service.isActiveModeReady(), "Unresolved Plan must not be sent as Agent"); + }); + assertFalse(actionsReady.get()); + + modes.complete(new ConversationMode[] {builtInMode("delayed-plan", "Plan")}); + awaitUi(() -> actionsReady.get() && resolvedModeNotification.get()); + runOnUi(() -> { + assertEquals("Plan", service.getActiveModeNameOrId()); + assertEquals("delayed-plan", + BuiltInChatModeManager.INSTANCE.getBuiltInModeByDisplayName("Plan").getId()); + }); + } finally { + broker.unsubscribe(listener); + runOnUi(() -> { + if (readinessBinding.get() != null) { + readinessBinding.get().dispose(); + } + }); + modes.complete(new ConversationMode[0]); + } + } + + @ParameterizedTest + @ValueSource(booleans = {false, true}) + void testModeDiscovery_FailedOrEmpty_ShowsRetryWithoutReloadingReadyPreferences(boolean empty) throws Exception { + CompletableFuture initial = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(initial); + Path file = writePreferences("{\"chatModeName\":\"Plan\"}"); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> "Plan".equals(service.getActiveModeNameOrId())); + UserPreference restored = storage.getReadyPreferences(); + runOnUi(() -> { + assertTrue(status.getVisible()); + assertEquals(Messages.modeDiscoveryLoading, statusMessage().getText()); + assertFalse(service.isActiveModeReady()); + }); + if (empty) { + initial.complete(new ConversationMode[0]); + } else { + initial.completeExceptionally(new IllegalStateException("mode discovery failed")); + } + awaitUi(() -> Messages.modeDiscoveryFailed.equals(statusMessage().getText())); + + CompletableFuture retry = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(retry); + runOnUi(() -> { + assertEquals(PreferenceStorage.State.READY, storage.getState()); + assertTrue(status.getVisible()); + Link retryLink = Arrays.stream(status.getChildren()).filter(Link.class::isInstance) + .map(Link.class::cast).findFirst().orElseThrow(); + assertTrue(retryLink.getEnabled()); + retryLink.notifyListeners(SWT.Selection, new org.eclipse.swt.widgets.Event()); + service.retryModeDiscovery(); + service.retryModeDiscovery(); + assertFalse(service.isActiveModeReady()); + }); + awaitUi(() -> Messages.modeDiscoveryLoading.equals(statusMessage().getText())); + runOnUi(() -> { + Link retryLink = Arrays.stream(status.getChildren()).filter(Link.class::isInstance) + .map(Link.class::cast).findFirst().orElseThrow(); + assertFalse(retryLink.getEnabled()); + }); + verify(connection, timeout(5000).times(2)).listConversationModes(any()); + retry.complete(new ConversationMode[] {builtInMode("recovered-plan", "Plan")}); + awaitUi(() -> service.isActiveModeReady() && !status.getVisible()); + runOnUi(() -> { + assertSame(restored, storage.getReadyPreferences()); + assertEquals("Plan", service.getActiveModeNameOrId()); + assertEquals("recovered-plan", + BuiltInChatModeManager.INSTANCE.getBuiltInModeByDisplayName("Plan").getId()); + }); + verify(connection).persistence(); + assertEquals("{\"chatModeName\":\"Plan\"}", Files.readString(file)); + } + + @Test + void testModeDiscovery_SupersededAttempt_CannotReplaceRecoveredInventory() throws Exception { + CompletableFuture obsolete = new CompletableFuture<>(); + CompletableFuture current = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(obsolete, current); + writePreferences("{\"chatModeName\":\"Plan\"}"); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> "Plan".equals(service.getActiveModeNameOrId())); + verify(connection, timeout(5000)).listConversationModes(any()); + UserPreference restored = storage.getReadyPreferences(); + + CopilotStatusResult statusResult = new CopilotStatusResult(); + statusResult.setStatus(CopilotStatusResult.OK); + statusResult.setUser("user"); + IEventBroker broker = PlatformUI.getWorkbench().getService(IEventBroker.class); + runOnUi(() -> { + broker.send(CopilotEventConstants.TOPIC_AUTH_STATUS_CHANGED, statusResult); + service.retryModeDiscovery(); + }); + verify(connection, timeout(5000).times(2)).listConversationModes(any()); + current.complete(new ConversationMode[] {builtInMode("current-plan", "Plan")}); + awaitUi(() -> service.isActiveModeReady() && !status.getVisible()); + obsolete.complete(new ConversationMode[] {builtInMode("obsolete-plan", "Plan")}); + runOnUi(() -> { + assertEquals("current-plan", BuiltInChatModeManager.INSTANCE.getBuiltInModeByDisplayName("Plan").getId()); + assertEquals(UserPreferenceService.ModeDiscoveryState.READY, service.getModeDiscoveryState()); + assertSame(restored, storage.getReadyPreferences()); + }); + verify(connection).persistence(); + } + + @Test + void testModeDiscovery_DisposedDuringManualRetry_RejectsLateSuccess() throws Exception { + when(connection.listConversationModes(any())) + .thenReturn(CompletableFuture.failedFuture(new IllegalStateException("offline"))); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + awaitUi(() -> service.getModeDiscoveryState() == UserPreferenceService.ModeDiscoveryState.FAILED); + CompletableFuture retry = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(retry); + runOnUi(service::retryModeDiscovery); + verify(connection, timeout(5000).times(2)).listConversationModes(any()); + runOnUi(() -> { + shell.dispose(); + service.dispose(); + }); + retry.complete(new ConversationMode[] {builtInMode("disposed-retry-mode", "Plan")}); + runOnUi(() -> { + assertFalse(service.isActiveModeReady()); + assertEquals(UserPreferenceService.ModeDiscoveryState.UNAVAILABLE, service.getModeDiscoveryState()); + assertNull(BuiltInChatModeManager.INSTANCE.getBuiltInModeById("disposed-retry-mode")); + }); + } + + @Test + void testModeDiscovery_AccountChanges_RejectsOldAccountModes() throws Exception { + CompletableFuture modes = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(modes); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + verify(connection, timeout(5000)).listConversationModes(any()); + awaitUi(() -> picker.getEnabled()); + + when(auth.getUserName()).thenReturn("other-user"); + modes.complete(new ConversationMode[] {builtInMode("obsolete-account-mode", "Ask")}); + runOnUi(() -> { + assertNull(BuiltInChatModeManager.INSTANCE.getBuiltInModeById("obsolete-account-mode")); + assertNull(service.getActiveModeNameOrId()); + }); + } + + @Test + void testModeDiscovery_DisposedService_DoesNotPublishLateModes() throws Exception { + CompletableFuture modes = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(modes); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + verify(connection, timeout(5000)).listConversationModes(any()); + runOnUi(() -> { + shell.dispose(); + service.dispose(); + }); + + modes.complete(new ConversationMode[] {builtInMode("disposed-service-mode", "Ask")}); + runOnUi(() -> + assertNull(BuiltInChatModeManager.INSTANCE.getBuiltInModeById("disposed-service-mode"))); + } + + @Test + void testModeDiscovery_AgentPolicyDisabled_KeepsAskViewAfterDelayedDiscovery() throws Exception { + CompletableFuture modes = new CompletableFuture<>(); + when(connection.listConversationModes(any())).thenReturn(modes); + writePreferences("{\"chatModeName\":\"Agent\"}"); + FeatureFlags flags = CopilotCore.getPlugin().getFeatureFlags(); + boolean original = flags.isAgentModeEnabled(); + try { + flags.setAgentModeEnabled(false); + startAuthenticated(CompletableFuture.completedFuture(persistence())); + verify(connection, timeout(5000)).listConversationModes(any()); + awaitUi(() -> picker.getEnabled()); + modes.complete(new ConversationMode[] { + builtInMode("delayed-policy-agent", "Agent"), builtInMode("delayed-policy-ask", "Ask")}); + awaitUi(() -> BuiltInChatModeManager.INSTANCE.getBuiltInModeById("delayed-policy-ask") != null); + runOnUi(() -> assertEquals(ChatMode.Ask, service.getActiveChatMode())); + } finally { + flags.setAgentModeEnabled(original); + } + } + @Test void testInitialization_RestoresModeHistoryAndConfirmationWithoutSaving() throws Exception { String saved = """ @@ -91,8 +366,8 @@ void testInitialization_RestoresModeHistoryAndConfirmationWithoutSaving() throws """; Path file = writePreferences(saved); startAuthenticated(CompletableFuture.completedFuture(persistence())); - awaitUi(() -> "Ask".equals(service.getActiveModeNameOrId())); - Display.getDefault().syncExec(() -> { + awaitUi(() -> "Ask".equals(service.getActiveModeNameOrId()) && !status.getVisible()); + runOnUi(() -> { assertEquals(ChatMode.Ask, service.getActiveChatMode()); assertEquals("second", service.getPreviousInput("")); assertEquals("first", service.getPreviousInput("")); @@ -112,7 +387,7 @@ void testInitialization_AgentPolicyDisabled_RestoresAskView() throws Exception { flags.setAgentModeEnabled(false); startAuthenticated(CompletableFuture.completedFuture(persistence())); awaitUi(() -> "Agent".equals(service.getActiveModeNameOrId())); - Display.getDefault().syncExec(() -> assertEquals(ChatMode.Ask, service.getActiveChatMode())); + runOnUi(() -> assertEquals(ChatMode.Ask, service.getActiveChatMode())); } finally { flags.setAgentModeEnabled(original); } @@ -123,7 +398,7 @@ void testRetry_FailedLoadKeepsPickerDisabledThenRestoresSavedChoice() throws Exc writePreferences("{\"chatModeName\":\"Ask\"}"); startAuthenticated(CompletableFuture.failedFuture(new IllegalStateException("offline"))); awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.FAILED); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { assertFalse(picker.getEnabled()); assertTrue(status.getVisible()); Label message = Arrays.stream(status.getChildren()).filter(Label.class::isInstance) @@ -133,7 +408,7 @@ void testRetry_FailedLoadKeepsPickerDisabledThenRestoresSavedChoice() throws Exc assertNull(service.getActiveModeNameOrId()); }); when(connection.persistence()).thenReturn(CompletableFuture.completedFuture(persistence())); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { Link retry = Arrays.stream(status.getChildren()).filter(Link.class::isInstance) .map(Link.class::cast).findFirst().orElseThrow(); assertTrue(retry.getEnabled()); @@ -154,7 +429,7 @@ void testSignOut_InvalidatesLoadedHistoryAndDisablesPicker() throws Exception { signedOut.setStatus(CopilotStatusResult.NOT_SIGNED_IN); listener.getValue().onDidCopilotStatusChange(signedOut); awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.UNAVAILABLE); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { assertFalse(picker.getEnabled()); assertNull(service.getActiveModeNameOrId()); assertEquals("", service.getPreviousInput("")); @@ -166,7 +441,7 @@ void testModeChange_AfterRestoration_UpdatesSharedStateAndPersists() throws Exce writePreferences("{\"chatModeName\":\"Ask\"}"); startAuthenticated(CompletableFuture.completedFuture(persistence())); awaitUi(() -> "Ask".equals(service.getActiveModeNameOrId())); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { service.setActiveChatMode("Agent"); assertEquals("Agent", service.getActiveModeNameOrId()); assertNotNull(storage.getReadyPreferences()); @@ -188,7 +463,7 @@ void testInitialization_DeadlineExpires_ShowsFailureAndRetryWithoutEnablingPicke return mock(ScheduledFuture.class); }); AtomicLong clock = new AtomicLong(); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { storage = new PreferenceStorage(connection, auth, DisplayRealm.getRealm(Display.getDefault()), Executors.newSingleThreadExecutor(), timer, clock::get, new PreferenceStorage.FileAccess() { @Override @@ -204,11 +479,11 @@ public void write(Path path, String content) throws IOException { createControls(); }); awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.LOADING); - Display.getDefault().syncExec(() -> assertFalse(picker.getEnabled())); + runOnUi(() -> assertFalse(picker.getEnabled())); clock.set(TimeUnit.SECONDS.toNanos(15)); deadline.get().run(); awaitUi(() -> storage.getReadiness().getValue() == PreferenceStorage.State.FAILED); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { assertFalse(picker.getEnabled()); assertTrue(status.getVisible()); Link retry = Arrays.stream(status.getChildren()).filter(Link.class::isInstance) @@ -221,7 +496,7 @@ private void startAuthenticated(CompletableFuture rpc) { when(auth.isSignedIn()).thenReturn(true); when(auth.getUserName()).thenReturn("user"); when(connection.persistence()).thenReturn(rpc); - Display.getDefault().syncExec(() -> { + runOnUi(() -> { storage = new PreferenceStorage(connection, auth); createControls(); }); @@ -230,7 +505,7 @@ private void startAuthenticated(CompletableFuture rpc) { private void createControls() { service = new UserPreferenceService(connection, auth, storage); shell = new Shell(Display.getDefault()); - status = new PreferenceStatus(shell, storage); + status = new PreferenceStatus(shell, storage, service); picker = new DropdownButton(shell, SWT.NONE); service.bindChatModePicker(picker); } @@ -241,6 +516,20 @@ private ChatPersistence persistence() { return result; } + private Label statusMessage() { + return Arrays.stream(status.getChildren()).filter(Label.class::isInstance) + .map(Label.class::cast).findFirst().orElseThrow(); + } + + private static ConversationMode builtInMode(String id, String name) { + ConversationMode mode = new ConversationMode(); + mode.setId(id); + mode.setName(name); + mode.setKind(name); + mode.setBuiltIn(true); + return mode; + } + private Path writePreferences(String content) throws Exception { Path file = directory.resolve("user").resolve("pref.json"); Files.createDirectories(file.getParent()); @@ -248,11 +537,25 @@ private Path writePreferences(String content) throws Exception { return file; } + private static void runOnUi(Runnable action) { + AtomicReference failure = new AtomicReference<>(); + Display.getDefault().syncExec(() -> { + try { + action.run(); + } catch (Throwable error) { + failure.set(error); + } + }); + if (failure.get() != null) { + fail("UI action failed", failure.get()); + } + } + private static void awaitUi(BooleanSupplier condition) throws InterruptedException { AtomicBoolean complete = new AtomicBoolean(); long deadline = System.nanoTime() + java.util.concurrent.TimeUnit.SECONDS.toNanos(5); do { - Display.getDefault().syncExec(() -> complete.set(condition.getAsBoolean())); + runOnUi(() -> complete.set(condition.getAsBoolean())); if (complete.get()) { return; } diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java index 09d2386ed..16d8528f6 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ActionBar.java @@ -148,7 +148,8 @@ public ActionBar(Composite parent, int style, ChatServiceManager chatServiceMana this.setLayoutData(new GridData(SWT.FILL, SWT.FILL, true, false)); this.setData(CssConstants.CSS_ID_KEY, "chat-action-bar-wrapper"); this.chatServiceManager = chatServiceManager; - new PreferenceStatus(this, chatServiceManager.getPreferenceStorage()); + new PreferenceStatus(this, chatServiceManager.getPreferenceStorage(), + chatServiceManager.getUserPreferenceService()); this.updateSendButtonToCancelButtonHandler = event -> { updateButtonState(SendOrCancelButtonStates.CANCEL_ENABLED); }; @@ -343,11 +344,13 @@ private void updateTableLayout(Table table) { updateButtonsLayout(); PreferenceStorage storage = chatServiceManager.getPreferenceStorage(); Realm.runWithDefault(storage.getReadiness().getRealm(), () -> { - ISideEffect readinessEffect = ISideEffect.create(storage.getReadiness()::getValue, state -> { + ISideEffect readinessEffect = ISideEffect.create(() -> { + return storage.getReadiness().getValue() == PreferenceStorage.State.READY + && chatServiceManager.getUserPreferenceService().isActiveModeReady(); + }, ready -> { if (isDisposed()) { return; } - boolean ready = state == PreferenceStorage.State.READY; mcpToolButton.setEnabled(ready); autoBreakpointButton.setEnabled(ready); if (isSendButton) { @@ -911,7 +914,8 @@ private void updateButtonState(SendOrCancelButtonStates state) { } private boolean preferencesReady() { - return chatServiceManager.getPreferenceStorage().getState() == PreferenceStorage.State.READY; + return chatServiceManager.getPreferenceStorage().getState() == PreferenceStorage.State.READY + && chatServiceManager.getUserPreferenceService().isActiveModeReady(); } /** diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java index 7db63670f..71ff46920 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ChatView.java @@ -1030,7 +1030,8 @@ public void setFocus() { private void onSendInternal(String workDoneToken, String message, String agentSlug, String agentJobWorkspaceFolder, boolean createNewTurn) { - if (chatServiceManager.getPreferenceStorage().getState() != PreferenceStorage.State.READY) { + if (chatServiceManager.getPreferenceStorage().getState() != PreferenceStorage.State.READY + || !chatServiceManager.getUserPreferenceService().isActiveModeReady()) { CopilotCore.LOGGER.error(new IllegalStateException("Cannot send chat before preferences are ready")); if (actionBar != null && !actionBar.isDisposed()) { actionBar.resetSendButton(); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java index cdec33180..9e911649a 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/Messages.java @@ -25,6 +25,8 @@ public final class Messages extends NLS { public static String preferenceLoadFailed; public static String preferenceUnavailable; public static String preferenceRetry; + public static String modeDiscoveryLoading; + public static String modeDiscoveryFailed; public static String agentMessageWidget_openInBrowserButton; public static String agentMessageWidget_openInBrowserTooltip; public static String agentMessageWidget_openJobListButton; diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java index ad069e55c..7e2766421 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/PreferenceStatus.java @@ -14,6 +14,8 @@ import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage; import com.microsoft.copilot.eclipse.ui.chat.services.PreferenceStorage.State; +import com.microsoft.copilot.eclipse.ui.chat.services.UserPreferenceService; +import com.microsoft.copilot.eclipse.ui.chat.services.UserPreferenceService.ModeDiscoveryState; /** * Inline preference-loading status, independent of model and conversation loading. @@ -26,6 +28,17 @@ public class PreferenceStatus extends Composite { * @param storage shared chat preference storage */ public PreferenceStatus(Composite parent, PreferenceStorage storage) { + this(parent, storage, null); + } + + /** + * Creates status and retry controls for preference loading and independent mode discovery. + * + * @param parent parent control + * @param storage shared chat preference storage + * @param preferences mode-discovery owner, or {@code null} for preference-only status + */ + public PreferenceStatus(Composite parent, PreferenceStorage storage, UserPreferenceService preferences) { super(parent, SWT.NONE); setLayout(new GridLayout(2, false)); GridData data = new GridData(SWT.FILL, SWT.CENTER, true, false); @@ -38,21 +51,38 @@ public PreferenceStatus(Composite parent, PreferenceStorage storage) { retry.setData("org.eclipse.swtbot.widget.key", "preference-retry"); GridData retryData = new GridData(SWT.RIGHT, SWT.CENTER, false, false); retry.setLayoutData(retryData); - retry.addListener(SWT.Selection, event -> storage.retry()); + retry.addListener(SWT.Selection, event -> { + if (storage.getState() == State.READY && preferences != null) { + preferences.retryModeDiscovery(); + } else { + storage.retry(); + } + }); Realm.runWithDefault(storage.getReadiness().getRealm(), () -> { - ISideEffect effect = ISideEffect.create(storage.getReadiness()::getValue, state -> { + ISideEffect effect = ISideEffect.create(() -> { + return new Readiness(storage.getReadiness().getValue(), + preferences == null ? ModeDiscoveryState.READY : preferences.getModeDiscoveryState()); + }, readiness -> { if (isDisposed()) { return; } - boolean visible = state != State.READY && state != State.DISPOSED; + State state = readiness.preferences(); + boolean modePending = state == State.READY && readiness.modes() != ModeDiscoveryState.READY; + boolean visible = state != State.DISPOSED && (state != State.READY || modePending); data.exclude = !visible; setVisible(visible); - message.setText(switch (state) { + String text = switch (state) { case LOADING -> Messages.preferenceLoading; case FAILED -> Messages.preferenceLoadFailed; default -> Messages.preferenceUnavailable; - }); - boolean canRetry = state == State.FAILED || state == State.UNAVAILABLE; + }; + if (modePending) { + text = readiness.modes() == ModeDiscoveryState.LOADING + ? Messages.modeDiscoveryLoading : Messages.modeDiscoveryFailed; + } + message.setText(text); + boolean canRetry = state == State.FAILED || state == State.UNAVAILABLE + || (modePending && readiness.modes() != ModeDiscoveryState.LOADING); retryData.exclude = !canRetry; retry.setVisible(canRetry); retry.setEnabled(canRetry); @@ -61,4 +91,7 @@ public PreferenceStatus(Composite parent, PreferenceStorage storage) { addDisposeListener(event -> effect.dispose()); }); } + + private record Readiness(State preferences, ModeDiscoveryState modes) { + } } diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFile.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFile.java index 050d5d487..19668f85c 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFile.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/ReferencedFile.java @@ -27,6 +27,7 @@ import org.eclipse.ui.model.WorkbenchLabelProvider; import com.microsoft.copilot.eclipse.core.Constants; +import com.microsoft.copilot.eclipse.core.lsp.protocol.CopilotModel; import com.microsoft.copilot.eclipse.ui.CopilotImages; import com.microsoft.copilot.eclipse.ui.CopilotUi; import com.microsoft.copilot.eclipse.ui.chat.services.ReferencedFileService; @@ -197,10 +198,9 @@ public void getName(AccessibleEvent event) { private void setupUnsupportedFileDisplay() { // Set warning icon lblfileIcon.setImage(CopilotImages.getSharedImage(ISharedImages.IMG_OBJS_WARN_TSK)); - // Set tooltip with model name - String modelName = CopilotUi.getPlugin().getChatServiceManager().getModelService().getActiveModel() - .getModelName(); - String tooltipText = String.format(Messages.chat_referencedFile_noVision_tooltip, modelName); + CopilotModel model = CopilotUi.getPlugin().getChatServiceManager().getModelService().getActiveModel(); + String tooltipText = model == null ? Messages.chat_referencedFile_modelUnavailable_tooltip + : String.format(Messages.chat_referencedFile_noVision_tooltip, model.getModelName()); lblfileIcon.setToolTipText(tooltipText); lblFileName.setToolTipText(tooltipText); lblClose.setToolTipText(tooltipText); diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties index 9e0222df8..43aa47fa8 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/messages.properties @@ -82,3 +82,5 @@ preferenceLoading=Loading chat preferences... preferenceLoadFailed=Could not load chat preferences. Retry to restore your saved settings. preferenceUnavailable=Chat preferences are unavailable. preferenceRetry=Retry +modeDiscoveryLoading=Loading chat modes... +modeDiscoveryFailed=Could not load chat modes. Retry to use your selected mode. diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java index 7e259d4cd..c612e8163 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/ModelService.java @@ -462,7 +462,11 @@ public void setActiveModel(String modelName) { * @return the active model */ public CopilotModel getActiveModel() { - return getUserPreference() == null ? null : activeModelObservable.getValue(); + if (disposed) { + return null; + } + CopilotModel model = activeModelObservable.getValue(); + return getUserPreference() == null ? null : model; } /** diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java index 796841f16..d2222d9c2 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/chat/services/PreferenceStorage.java @@ -349,8 +349,9 @@ private void read(Attempt pending, ChatPersistence result) { } private UserPreference parse(String json) throws IOException { + rejectLegacyJsonExtensions(json); try (JsonReader reader = new JsonReader(new StringReader(json))) { - configureStrictJson(reader); + reader.setLenient(false); if (reader.peek() != JsonToken.BEGIN_OBJECT) { throw new JsonSyntaxException("Preferences must contain an object"); } @@ -365,15 +366,36 @@ private UserPreference parse(String json) throws IOException { } } - private void configureStrictJson(JsonReader reader) throws IOException { - try { - // Older supported Eclipse targets bundle Gson 2.10, before the Strictness API. - Class strictness = Class.forName("com.google.gson.Strictness"); - JsonReader.class.getMethod("setStrictness", strictness).invoke(reader, strictness.getField("STRICT").get(null)); - } catch (ClassNotFoundException | NoSuchMethodException exception) { - reader.setLenient(false); - } catch (ReflectiveOperationException exception) { - throw new IOException("Cannot configure strict preference JSON parsing", exception); + private void rejectLegacyJsonExtensions(String json) { + // Gson 2.10's non-lenient mode still accepts control characters, non-JSON escapes and mixed-case literals. + // Reject those extensions before the reader validates the document's remaining syntax and types. + boolean quoted = false; + for (int index = 0; index < json.length(); index++) { + char character = json.charAt(index); + if (quoted) { + if (character < 0x20) { + throw new JsonSyntaxException("Unescaped control character in preferences"); + } + if (character == '\\') { + index++; + if (index == json.length() || "\"\\/bfnrtu".indexOf(json.charAt(index)) < 0) { + throw new JsonSyntaxException("Invalid escape in preferences"); + } + } else if (character == '"') { + quoted = false; + } + } else if (character == '"') { + quoted = true; + } else if ("tTfFnN".indexOf(character) >= 0) { + int start = index; + while (index + 1 < json.length() && Character.isLetter(json.charAt(index + 1))) { + index++; + } + String literal = json.substring(start, index + 1); + if (!"true".equals(literal) && !"false".equals(literal) && !"null".equals(literal)) { + throw new JsonSyntaxException("Invalid literal in preferences"); + } + } } } 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 cc6a793de..d7e9f1397 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 @@ -9,6 +9,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.concurrent.CompletableFuture; import org.apache.commons.lang3.StringUtils; import org.eclipse.core.databinding.observable.sideeffect.ISideEffect; @@ -29,6 +30,7 @@ import com.microsoft.copilot.eclipse.core.chat.CustomChatModeManager; import com.microsoft.copilot.eclipse.core.chat.InputNavigation; import com.microsoft.copilot.eclipse.core.chat.UserPreference; +import com.microsoft.copilot.eclipse.core.chat.service.BuiltInChatModeService; import com.microsoft.copilot.eclipse.core.events.CopilotEventConstants; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatMode; @@ -47,9 +49,19 @@ * Service for managing chat modes and input navigation. */ public class UserPreferenceService extends ChatBaseService { + /** + * Availability of the independently discovered built-in mode inventory. + */ + public enum ModeDiscoveryState { + UNAVAILABLE, LOADING, READY, FAILED + } + private final PreferenceStorage preferenceStorage; private ISideEffect readinessSideEffect; private volatile boolean disposed; + private long modeDiscoveryGeneration; + private CompletableFuture> modeDiscovery; + private IObservableValue modeDiscoveryState; 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 @@ -76,6 +88,7 @@ public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthS chatModeObservable = new WritableValue<>(getAvailableChatModes(), String[].class); activeChatModeObservable = new WritableValue<>(null, ChatMode.class); activeModeNameOrIdObservable = new WritableValue<>(null, String.class); + modeDiscoveryState = new WritableValue<>(ModeDiscoveryState.UNAVAILABLE, ModeDiscoveryState.class); readinessSideEffect = ISideEffect.create(preferenceStorage.getReadiness()::getValue, state -> { if (state == PreferenceStorage.State.READY) { init(); @@ -90,6 +103,7 @@ public UserPreferenceService(CopilotLanguageServerConnection lsConnection, AuthS initializeEventHandlers(); subscribeToEvents(); preferenceStorage.initialize(); + reloadBuiltInModes(); } private void initializeEventHandlers() { @@ -98,19 +112,9 @@ private void initializeEventHandlers() { if (property instanceof CopilotStatusResult statusResult) { if (statusResult.isSignedIn() && authStatusManager.isSignedIn() && Objects.equals(statusResult.getUser(), authStatusManager.getUserName())) { - // 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()); - } - }); - } catch (Exception e) { - CopilotCore.LOGGER.error("Failed to reload built-in modes on user switch", e); - } + reloadBuiltInModes(); + } else if (!statusResult.isSignedIn()) { + ensureRealm(this::cancelModeDiscovery); } } }; @@ -131,6 +135,55 @@ private void initializeEventHandlers() { }; } + private void reloadBuiltInModes() { + ensureRealm(() -> { + cancelModeDiscovery(); + if (!authStatusManager.isSignedIn()) { + return; + } + long generation = modeDiscoveryGeneration; + String account = authStatusManager.getUserName(); + modeDiscoveryState.setValue(ModeDiscoveryState.LOADING); + CompletableFuture> discovery = CompletableFuture + .supplyAsync(() -> new BuiltInChatModeService().loadBuiltInModes(lsConnection)) + .thenCompose(result -> result); + modeDiscovery = discovery; + discovery.thenAccept(modes -> ensureRealm(() -> { + if (generation != modeDiscoveryGeneration || !authStatusManager.isSignedIn() + || !Objects.equals(account, authStatusManager.getUserName())) { + return; + } + if (modes.isEmpty()) { + modeDiscoveryState.setValue(ModeDiscoveryState.FAILED); + return; + } + BuiltInChatModeManager.INSTANCE.updateModes(modes); + chatModeObservable.setValue(getAvailableChatModes()); + modeDiscoveryState.setValue(ModeDiscoveryState.READY); + if (eventBroker != null && getUserPreference() != null) { + eventBroker.post(CopilotEventConstants.TOPIC_CHAT_MODE_CHANGED, getActiveChatMode()); + } + })).exceptionally(exception -> { + ensureRealm(() -> { + if (generation == modeDiscoveryGeneration && authStatusManager.isSignedIn() + && Objects.equals(account, authStatusManager.getUserName())) { + modeDiscoveryState.setValue(ModeDiscoveryState.FAILED); + CopilotCore.LOGGER.error("Failed to reload built-in modes", exception); + } + }); + return null; + }); + }); + } + + private void cancelModeDiscovery() { + modeDiscoveryGeneration++; + modeDiscoveryState.setValue(ModeDiscoveryState.UNAVAILABLE); + if (modeDiscovery != null) { + modeDiscovery.cancel(true); + } + } + private void subscribeToEvents() { eventBroker = PlatformUI.getWorkbench().getService(IEventBroker.class); if (eventBroker != null) { @@ -325,6 +378,47 @@ public String getActiveModeNameOrId() { return getUserPreference() == null ? null : activeModeNameOrIdObservable.getValue(); } + /** + * Returns whether the active preference can be resolved without falling back to another chat mode. + * + * @return whether actions depending on the selected mode can run + */ + public boolean isActiveModeReady() { + if (disposed) { + return false; + } + boolean inventoryReady = modeDiscoveryState.getValue() == ModeDiscoveryState.READY; + String modeName = activeModeNameOrIdObservable.getValue(); + if (getUserPreference() == null) { + return false; + } + if (CustomChatModeManager.INSTANCE.isCustomMode(modeName)) { + return CustomChatModeManager.INSTANCE.getCustomModeById(modeName) != null; + } + return inventoryReady && BuiltInChatModeManager.INSTANCE.getBuiltInModeByDisplayName(modeName) != null; + } + + /** + * Returns mode discovery readiness in the UI Realm, independently of preference-file loading. + * + * @return the current discovery state + */ + public ModeDiscoveryState getModeDiscoveryState() { + return disposed ? ModeDiscoveryState.UNAVAILABLE : modeDiscoveryState.getValue(); + } + + /** + * Retries failed or unavailable mode discovery without reloading ready preferences. + */ + public void retryModeDiscovery() { + ensureRealm(() -> { + ModeDiscoveryState state = modeDiscoveryState.getValue(); + if (state == ModeDiscoveryState.FAILED || state == ModeDiscoveryState.UNAVAILABLE) { + reloadBuiltInModes(); + } + }); + } + /** * Get the active custom mode if one is selected. * @@ -609,6 +703,7 @@ public void dispose() { persistUserPreference(); disposed = true; super.ensureRealm(() -> { + cancelModeDiscovery(); readinessSideEffect.dispose(); unbindChatView(); for (DropdownButton picker : List.copyOf(chatModeButtonSideEffects.keySet())) { @@ -617,6 +712,7 @@ public void dispose() { chatModeObservable.dispose(); activeChatModeObservable.dispose(); activeModeNameOrIdObservable.dispose(); + modeDiscoveryState.dispose(); }); if (eventBroker != null) { diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/Messages.java b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/Messages.java index 3f16e3321..f75dc5524 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/Messages.java +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/Messages.java @@ -169,6 +169,7 @@ public final class Messages extends NLS { public static String chat_customModels; public static String chat_addPremiumModels; public static String chat_referencedFile_noVision_tooltip; + public static String chat_referencedFile_modelUnavailable_tooltip; public static String agent_tool_compareEditor_titlePrefix; public static String agent_tool_compareEditor_proposedChangesTitle; public static String agentFileEditor_contentAssist_statusMessage; diff --git a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/messages.properties b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/messages.properties index f377e3108..453e84437 100644 --- a/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/messages.properties +++ b/com.microsoft.copilot.eclipse.ui/src/com/microsoft/copilot/eclipse/ui/i18n/messages.properties @@ -171,6 +171,7 @@ chat_premiumModels=Premium Models chat_customModels=Custom Models chat_addPremiumModels=Add Premium Models chat_referencedFile_noVision_tooltip=%s does not support images. +chat_referencedFile_modelUnavailable_tooltip=Images are unavailable until a model is ready. agent_tool_compareEditor_titlePrefix=Changes from GitHub Copilot: agent_tool_compareEditor_proposedChangesTitle=Proposed Changes From 7734c138d9369bf37b6090be4c864af579fa4931 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 30 Sep 2026 02:52:56 +0000 Subject: [PATCH 3/4] Initial plan From 39bfe438f5abdd65b4215af4f98b6bf30d871975 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 30 Sep 2026 02:57:28 +0000 Subject: [PATCH 4/4] Refactor built-in mode discovery manager Co-authored-by: jdneo <6193897+jdneo@users.noreply.github.com> --- .../chat/BuiltInChatModeManagerTests.java | 142 ++++++++++++++++++ .../service/BuiltInChatModeServiceTests.java | 75 --------- .../core/chat/BuiltInChatModeManager.java | 57 +++++++ .../chat/service/BuiltInChatModeService.java | 81 ---------- .../chat/services/UserPreferenceService.java | 3 +- 5 files changed, 200 insertions(+), 158 deletions(-) create mode 100644 com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java delete mode 100644 com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeServiceTests.java delete mode 100644 com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.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 000000000..3368e6af2 --- /dev/null +++ b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/BuiltInChatModeManagerTests.java @@ -0,0 +1,142 @@ +// 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.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verifyNoInteractions; +import static org.mockito.Mockito.when; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionException; + +import org.junit.jupiter.api.AfterEach; +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.lsp.CopilotLanguageServerConnection; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ConversationMode; +import com.microsoft.copilot.eclipse.core.lsp.protocol.ConversationModesParams; + +@ExtendWith(MockitoExtension.class) +class BuiltInChatModeManagerTests { + + @Mock + private CopilotLanguageServerConnection mockConnection; + + @BeforeEach + void setUp() { + BuiltInChatModeManager.INSTANCE.updateModes(List.of()); + } + + @AfterEach + void clearSnapshot() { + BuiltInChatModeManager.INSTANCE.updateModes(List.of()); + } + + @Test + void testLoadBuiltInModes_inlineAgentSkippedFromAgentModes() { + ConversationMode agentMode = createConversationMode("Agent", "Agent", "Agent"); + ConversationMode inlineAgentMode = createConversationMode("InlineAgent", "Agent", "InlineAgent"); + + when(mockConnection.listConversationModes(any(ConversationModesParams.class))) + .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { agentMode, inlineAgentMode })); + + List builtInModes = BuiltInChatModeManager.INSTANCE.loadBuiltInModes(mockConnection).join(); + + assertEquals(1, builtInModes.size()); + BuiltInChatMode builtInMode = builtInModes.get(0); + assertEquals("Agent", builtInMode.getId()); + assertEquals("Agent", builtInMode.getDisplayName()); + assertEquals("Agent", builtInMode.getKind()); + } + + @Test + void testLoadBuiltInModes_successDoesNotPublishSnapshot() { + BuiltInChatMode publishedMode = createBuiltInChatMode("Ask"); + BuiltInChatModeManager.INSTANCE.updateModes(List.of(publishedMode)); + ConversationMode loadedMode = createConversationMode("Agent", "Agent", "Agent"); + when(mockConnection.listConversationModes(any(ConversationModesParams.class))) + .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { loadedMode })); + + List result = BuiltInChatModeManager.INSTANCE.loadBuiltInModes(mockConnection).join(); + + assertEquals(List.of("Agent"), result.stream().map(BuiltInChatMode::getId).toList()); + assertEquals(List.of(publishedMode), BuiltInChatModeManager.INSTANCE.getBuiltInModes()); + } + + @Test + void testLoadBuiltInModes_failureDoesNotReplaceSnapshot() { + BuiltInChatMode publishedMode = createBuiltInChatMode("Ask"); + BuiltInChatModeManager.INSTANCE.updateModes(List.of(publishedMode)); + IllegalStateException failure = new IllegalStateException("mode discovery failed"); + when(mockConnection.listConversationModes(any(ConversationModesParams.class))) + .thenReturn(CompletableFuture.failedFuture(failure)); + + CompletionException exception = assertThrows(CompletionException.class, + () -> BuiltInChatModeManager.INSTANCE.loadBuiltInModes(mockConnection).join()); + + assertSame(failure, exception.getCause()); + assertEquals(List.of(publishedMode), BuiltInChatModeManager.INSTANCE.getBuiltInModes()); + } + + @Test + void testLoadBuiltInModes_nullConnectionReturnsEmptyWithoutPublishing() { + BuiltInChatMode publishedMode = createBuiltInChatMode("Ask"); + BuiltInChatModeManager.INSTANCE.updateModes(List.of(publishedMode)); + + assertEquals(List.of(), BuiltInChatModeManager.INSTANCE.loadBuiltInModes(null).join()); + assertEquals(List.of(publishedMode), BuiltInChatModeManager.INSTANCE.getBuiltInModes()); + verifyNoInteractions(mockConnection); + } + + @Test + void testLoadBuiltInModes_conversionFailureSkipsMode() { + ConversationMode invalidMode = mock(ConversationMode.class); + when(invalidMode.isBuiltIn()).thenReturn(true); + when(invalidMode.getKind()).thenReturn("Agent"); + when(invalidMode.getName()).thenReturn("Agent"); + when(invalidMode.getId()).thenReturn("invalid"); + when(invalidMode.getDescription()).thenThrow(new IllegalArgumentException("invalid mode")); + when(mockConnection.listConversationModes(any(ConversationModesParams.class))) + .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { invalidMode })); + + assertEquals(List.of(), BuiltInChatModeManager.INSTANCE.loadBuiltInModes(mockConnection).join()); + } + + @Test + void testUpdateModes_publishesDefensiveSnapshot() { + BuiltInChatMode mode = createBuiltInChatMode("Plan"); + List source = new ArrayList<>(List.of(mode)); + + BuiltInChatModeManager.INSTANCE.updateModes(source); + source.clear(); + List snapshot = BuiltInChatModeManager.INSTANCE.getBuiltInModes(); + snapshot.clear(); + + assertEquals(List.of(mode), BuiltInChatModeManager.INSTANCE.getBuiltInModes()); + } + + private BuiltInChatMode createBuiltInChatMode(String name) { + return new BuiltInChatMode(createConversationMode(name, name, name)); + } + + private ConversationMode createConversationMode(String id, String name, String kind) { + ConversationMode mode = new ConversationMode(); + mode.setId(id); + mode.setName(name); + mode.setKind(kind); + mode.setBuiltIn(true); + mode.setDescription(name + " description"); + return mode; + } +} diff --git a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeServiceTests.java b/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeServiceTests.java deleted file mode 100644 index 22039b462..000000000 --- a/com.microsoft.copilot.eclipse.core.test/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeServiceTests.java +++ /dev/null @@ -1,75 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -package com.microsoft.copilot.eclipse.core.chat.service; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.when; - -import java.lang.reflect.Field; -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.CopilotCore; -import com.microsoft.copilot.eclipse.core.chat.BuiltInChatMode; -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; - -@ExtendWith(MockitoExtension.class) -class BuiltInChatModeServiceTests { - - @Mock - private CopilotLanguageServerConnection mockConnection; - - private BuiltInChatModeService builtInChatModeService; - - @BeforeEach - void setUp() throws Exception { - builtInChatModeService = new BuiltInChatModeService(); - - CopilotCore plugin = new CopilotCore(); - Field languageServerField = CopilotCore.class.getDeclaredField("copilotLanguageServer"); - languageServerField.setAccessible(true); - languageServerField.set(plugin, mockConnection); - } - - @Test - void testLoadBuiltInModes_inlineAgentSkippedFromAgentModes() { - ConversationMode agentMode = createBuiltInMode("Agent", "Agent", "Agent", - "Advanced agent mode with access to tools and capabilities"); - ConversationMode inlineAgentMode = createBuiltInMode("InlineAgent", "Agent", "InlineAgent", - "Agent mode with a restricted tool set for inline editing"); - - when(mockConnection.listConversationModes(any(ConversationModesParams.class))) - .thenReturn(CompletableFuture.completedFuture(new ConversationMode[] { agentMode, inlineAgentMode })); - - List builtInModes = builtInChatModeService.loadBuiltInModes().join(); - - assertEquals(1, builtInModes.size()); - - BuiltInChatMode builtInMode = builtInModes.get(0); - assertNotNull(builtInMode); - assertEquals("Agent", builtInMode.getId()); - assertEquals("Agent", builtInMode.getDisplayName()); - assertEquals("Agent", builtInMode.getKind()); - } - - private ConversationMode createBuiltInMode(String id, String name, String kind, String description) { - ConversationMode mode = new ConversationMode(); - mode.setId(id); - mode.setName(name); - mode.setKind(kind); - mode.setBuiltIn(true); - mode.setDescription(description); - return 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 fd4199bbd..a634224fa 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 @@ -4,7 +4,15 @@ package com.microsoft.copilot.eclipse.core.chat; import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; import java.util.List; +import java.util.concurrent.CompletableFuture; + +import com.microsoft.copilot.eclipse.core.CopilotCore; +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; /** * Shared snapshot of built-in chat modes discovered asynchronously by the chat lifecycle. @@ -12,8 +20,48 @@ public enum BuiltInChatModeManager { INSTANCE; + private static final List ALLOWED_BUILTIN_NAMES = Arrays.asList(BuiltInChatMode.ASK_MODE_NAME, + BuiltInChatMode.AGENT_MODE_NAME, BuiltInChatMode.PLAN_MODE_NAME, BuiltInChatMode.DEBUGGER_MODE_NAME); + private volatile List builtInModes = List.of(); + /** + * Loads built-in modes using the owning chat lifecycle's language-server connection. + * + * @param lsConnection the connection used by the chat services + * @return the discovered built-in modes + */ + public CompletableFuture> loadBuiltInModes( + CopilotLanguageServerConnection lsConnection) { + if (lsConnection == null) { + return CompletableFuture.completedFuture(new ArrayList<>()); + } + ConversationModesParams params = new ConversationModesParams(Collections.emptyList()); + + return lsConnection.listConversationModes(params).thenApply(conversationModes -> { + List loadedModes = new ArrayList<>(); + + for (ConversationMode mode : conversationModes) { + if (mode == null || !mode.isBuiltIn()) { + continue; + } + // Exclude InlineAgent kind — it is not a user-facing chat mode + if (BuiltInChatMode.INLINE_AGENT_KIND.equalsIgnoreCase(mode.getKind())) { + continue; + } + // Filter to only allowed built-in modes by name (case-insensitive) + if (ALLOWED_BUILTIN_NAMES.stream().anyMatch(name -> name.equalsIgnoreCase(mode.getName()))) { + BuiltInChatMode builtInMode = convertToBuiltInChatMode(mode); + if (builtInMode != null) { + loadedModes.add(builtInMode); + } + } + } + + return loadedModes; + }); + } + public List getBuiltInModes() { return new ArrayList<>(builtInModes); } @@ -47,4 +95,13 @@ public BuiltInChatMode getBuiltInModeById(String id) { public void updateModes(List modes) { builtInModes = List.copyOf(modes); } + + private BuiltInChatMode convertToBuiltInChatMode(ConversationMode mode) { + try { + return new BuiltInChatMode(mode); + } catch (Exception e) { + CopilotCore.LOGGER.error("Failed to convert built-in mode: " + mode.getId(), e); + return null; + } + } } \ No newline at end of file diff --git a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java b/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java deleted file mode 100644 index e005f198c..000000000 --- a/com.microsoft.copilot.eclipse.core/src/com/microsoft/copilot/eclipse/core/chat/service/BuiltInChatModeService.java +++ /dev/null @@ -1,81 +0,0 @@ -// Copyright (c) Microsoft Corporation. -// Licensed under the MIT license. - -package com.microsoft.copilot.eclipse.core.chat.service; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.List; -import java.util.concurrent.CompletableFuture; - -import com.microsoft.copilot.eclipse.core.CopilotCore; -import com.microsoft.copilot.eclipse.core.chat.BuiltInChatMode; -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; - -/** - * Service for loading built-in chat modes from the LSP API. Built-in modes include Ask, Agent, and Plan. - */ -public class BuiltInChatModeService { - - private static final List ALLOWED_BUILTIN_NAMES = Arrays.asList(BuiltInChatMode.ASK_MODE_NAME, - BuiltInChatMode.AGENT_MODE_NAME, BuiltInChatMode.PLAN_MODE_NAME, BuiltInChatMode.DEBUGGER_MODE_NAME); - - /** - * Loads built-in modes from the LSP API. Only modes with names in ALLOWED_BUILTIN_NAMES are returned. - * - *

Note: The LSP requires workspace folders to be passed even for loading built-in modes. While built-in modes - * don't depend on workspace context, the LSP API enforces this parameter. - */ - public CompletableFuture> loadBuiltInModes() { - return loadBuiltInModes(CopilotCore.getPlugin().getCopilotLanguageServer()); - } - - /** - * Loads built-in modes using the owning chat lifecycle's language-server connection. - * - * @param lspConnection the connection used by the chat services - * @return the discovered built-in modes - */ - public CompletableFuture> loadBuiltInModes( - CopilotLanguageServerConnection lspConnection) { - if (lspConnection == null) { - return CompletableFuture.completedFuture(new ArrayList<>()); - } - ConversationModesParams params = new ConversationModesParams(Collections.emptyList()); - - return lspConnection.listConversationModes(params).thenApply(conversationModes -> { - List builtInModes = new ArrayList<>(); - - for (ConversationMode mode : conversationModes) { - if (mode == null || !mode.isBuiltIn()) { - continue; - } - // Exclude InlineAgent kind — it is not a user-facing chat mode - if (BuiltInChatMode.INLINE_AGENT_KIND.equalsIgnoreCase(mode.getKind())) { - continue; - } - // Filter to only allowed built-in modes by name (case-insensitive) - if (ALLOWED_BUILTIN_NAMES.stream().anyMatch(name -> name.equalsIgnoreCase(mode.getName()))) { - BuiltInChatMode builtIn = convertToBuiltInChatMode(mode); - if (builtIn != null) { - builtInModes.add(builtIn); - } - } - } - - return builtInModes; - }); - } - - private BuiltInChatMode convertToBuiltInChatMode(ConversationMode mode) { - try { - return new BuiltInChatMode(mode); - } catch (Exception e) { - CopilotCore.LOGGER.error("Failed to convert built-in mode: " + mode.getId(), e); - return null; - } - } -} \ 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 d7e9f1397..16ec1618b 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 @@ -30,7 +30,6 @@ import com.microsoft.copilot.eclipse.core.chat.CustomChatModeManager; import com.microsoft.copilot.eclipse.core.chat.InputNavigation; import com.microsoft.copilot.eclipse.core.chat.UserPreference; -import com.microsoft.copilot.eclipse.core.chat.service.BuiltInChatModeService; import com.microsoft.copilot.eclipse.core.events.CopilotEventConstants; import com.microsoft.copilot.eclipse.core.lsp.CopilotLanguageServerConnection; import com.microsoft.copilot.eclipse.core.lsp.protocol.ChatMode; @@ -145,7 +144,7 @@ private void reloadBuiltInModes() { String account = authStatusManager.getUserName(); modeDiscoveryState.setValue(ModeDiscoveryState.LOADING); CompletableFuture> discovery = CompletableFuture - .supplyAsync(() -> new BuiltInChatModeService().loadBuiltInModes(lsConnection)) + .supplyAsync(() -> BuiltInChatModeManager.INSTANCE.loadBuiltInModes(lsConnection)) .thenCompose(result -> result); modeDiscovery = discovery; discovery.thenAccept(modes -> ensureRealm(() -> {