diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/BookmarkManagerTests.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/BookmarkManagerTests.cs index 0751b5afe3..b4e533d66d 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/BookmarkManagerTests.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/BookmarkManagerTests.cs @@ -186,4 +186,48 @@ public class BookmarkManagerTests Assert.AreEqual("D:\\UpdatedPath", updatedBookmark.Bookmark); Assert.IsTrue(bookmarkUpdatedEventFired); } + + [TestMethod] + public void BookmarkManager_LegacyData_IdsArePersistedAcrossLoads() + { + // Arrange + const string json = """ + { + "Data": + [ + { "Name": "C:\\","Bookmark": "C:\\" }, + { "Name": "Bing.com","Bookmark": "https://bing.com" } + ] + } + """; + + var dataSource = new MockBookmarkDataSource(json); + + // First load: IDs should be generated for legacy entries + var manager1 = new BookmarksManager(dataSource); + var firstLoad = manager1.Bookmarks.ToList(); + Assert.AreEqual(2, firstLoad.Count); + Assert.AreNotEqual(Guid.Empty, firstLoad[0].Id); + Assert.AreNotEqual(Guid.Empty, firstLoad[1].Id); + + // Keep a name->id map to be insensitive to ordering + var firstIdsByName = firstLoad.ToDictionary(b => b.Name, b => b.Id); + + // Wait deterministically for async persistence to complete + var wasSaved = dataSource.WaitForSave(1, 5000); + Assert.IsTrue(wasSaved, "Data was not saved within the expected time."); + + // Second load: should read back the same IDs from persisted data + var manager2 = new BookmarksManager(dataSource); + var secondLoad = manager2.Bookmarks.ToList(); + Assert.AreEqual(2, secondLoad.Count); + + var secondIdsByName = secondLoad.ToDictionary(b => b.Name, b => b.Id); + + foreach (var kvp in firstIdsByName) + { + Assert.IsTrue(secondIdsByName.ContainsKey(kvp.Key), $"Missing bookmark '{kvp.Key}' after reload."); + Assert.AreEqual(kvp.Value, secondIdsByName[kvp.Key], $"Bookmark '{kvp.Key}' upgraded ID was not persisted across loads."); + } + } } diff --git a/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/MockBookmarkDataSource.cs b/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/MockBookmarkDataSource.cs index 3980ac13c6..02d71d6d77 100644 --- a/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/MockBookmarkDataSource.cs +++ b/src/modules/cmdpal/Tests/Microsoft.CmdPal.Ext.Bookmarks.UnitTests/MockBookmarkDataSource.cs @@ -2,6 +2,8 @@ // The Microsoft Corporation licenses this file to you under the MIT license. // See the LICENSE file in the project root for more information. +using System; +using System.Threading; using Microsoft.CmdPal.Ext.Bookmarks.Persistence; namespace Microsoft.CmdPal.Ext.Bookmarks.UnitTests; @@ -9,6 +11,7 @@ namespace Microsoft.CmdPal.Ext.Bookmarks.UnitTests; internal sealed class MockBookmarkDataSource : IBookmarkDataSource { private string _jsonData; + private int _saveCount; public MockBookmarkDataSource(string initialJsonData = "[]") { @@ -23,5 +26,26 @@ internal sealed class MockBookmarkDataSource : IBookmarkDataSource public void SaveBookmarkData(string jsonData) { _jsonData = jsonData; + Interlocked.Increment(ref _saveCount); + } + + public int SaveCount => Volatile.Read(ref _saveCount); + + // Waits until at least expectedMinSaves have occurred or the timeout elapses. + // Returns true if the condition was met, false on timeout. + public bool WaitForSave(int expectedMinSaves = 1, int timeoutMs = 2000) + { + var start = Environment.TickCount; + while (Volatile.Read(ref _saveCount) < expectedMinSaves) + { + if (Environment.TickCount - start > timeoutMs) + { + return false; + } + + Thread.Sleep(50); + } + + return true; } } diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/BookmarksManager.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/BookmarksManager.cs index 1eb57fb7eb..fde574360f 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/BookmarksManager.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/BookmarksManager.cs @@ -103,7 +103,33 @@ internal sealed partial class BookmarksManager : IDisposable, IBookmarksManager try { var jsonData = _dataSource.GetBookmarkData(); - _bookmarksData = _parser.ParseBookmarks(jsonData); + var bookmarksData = _parser.ParseBookmarks(jsonData); + + // Upgrade old bookmarks if necessary + // Pre .95 versions did not assign IDs to bookmarks + var upgraded = false; + for (var index = 0; index < bookmarksData.Data.Count; index++) + { + var bookmark = bookmarksData.Data[index]; + if (bookmark.Id == Guid.Empty) + { + bookmarksData.Data[index] = bookmark with { Id = Guid.NewGuid() }; + upgraded = true; + } + } + + lock (_lock) + { + _bookmarksData = bookmarksData; + } + + // LOAD BEARING: Save upgraded data back to file + // This ensures that old bookmarks are not repeatedly upgraded on each load, + // as the hotkeys and aliases are tied to the generated bookmark IDs. + if (upgraded) + { + _ = SaveChangesAsync(); + } } catch (Exception ex) { diff --git a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/Persistence/BookmarkData.cs b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/Persistence/BookmarkData.cs index 3129e1b578..b577f9cb35 100644 --- a/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/Persistence/BookmarkData.cs +++ b/src/modules/cmdpal/ext/Microsoft.CmdPal.Ext.Bookmark/Persistence/BookmarkData.cs @@ -19,7 +19,7 @@ public sealed record BookmarkData [SetsRequiredMembers] public BookmarkData(Guid id, string? name, string? bookmark) { - Id = id == Guid.Empty ? Guid.NewGuid() : id; + Id = id; Name = name ?? string.Empty; Bookmark = bookmark ?? string.Empty; }