From 13fed10c99a23e63e8884c0ad61d51aa7d17bca4 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 18:10:58 -0400 Subject: [PATCH 01/14] feat(workspace): replace single-root workspace with multi-source WorkspaceSourceSet Introduce WorkspaceSourceSet (WorkspaceSubsystem) to own an ordered set of file/folder sources and resolve them into a merged, deduplicated file list with first-registered-source-wins attribution. WorkspaceLoadOptions moves here from WorkspaceModel, which now loads directly from a WorkspaceSourceResolution instead of scanning a single root path. WorkspaceModel.ReloadFilesAsync gains an optional updatedResolution parameter so callers can re-resolve sources (picking up externally created/deleted files under a still-watched folder) before an incremental reload, while still preserving unaffected files' LoadedUtc. FileWatcher moves from a single global root watcher to a per-source watch scope (WatchSource/UnwatchSource keyed by source id): a Folder source gets one recursive watcher, a File source gets one filtered watcher scoped to just that file. Unwatching one source no longer discards other sources' pending change state. Adds WorkspaceSourceSetTests, rewrites WorkspaceModelTests/FileWatcherTests for the new APIs (including empty-resolution coverage), and updates the WorkspaceSubsystemTests integration test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../WorkspaceSubsystem/FileWatcher.cs | 136 ++++++++--- .../WorkspaceSubsystem/WorkspaceModel.cs | 122 +++++----- .../WorkspaceSubsystem/WorkspaceSourceSet.cs | 195 +++++++++++++++ .../WorkspaceSubsystem/FileWatcherTests.cs | 171 ++++++++++--- .../WorkspaceSubsystem/WorkspaceModelTests.cs | 77 ++++-- .../WorkspaceSourceSetTests.cs | 229 ++++++++++++++++++ .../WorkspaceSubsystemTests.cs | 16 +- 7 files changed, 790 insertions(+), 156 deletions(-) create mode 100644 src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs create mode 100644 test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs diff --git a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs index 04907b6..739abc3 100644 --- a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs +++ b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs @@ -60,9 +60,9 @@ public sealed class FileWatcher : IDisposable private readonly IUiDispatcher _dispatcher; /// - /// Operating-system watcher configured for recursive observation of the workspace root. + /// Operating-system watchers, one per currently watched source, keyed by . /// - private FileSystemWatcher? _watcher; + private readonly Dictionary _watchersBySourceId = new(StringComparer.Ordinal); /// /// Initializes a new . @@ -78,10 +78,9 @@ public FileWatcher(TimeSpan debounceWindow, Func? clock = null, } /// - /// Workspace root currently monitored for external changes, or before - /// is called. + /// Identifiers of every source currently being monitored for external changes. /// - public string? WatchedRootPath { get; private set; } + public IReadOnlySet WatchedSourceIds => _watchersBySourceId.Keys.ToHashSet(StringComparer.Ordinal); /// /// Minimum delay used to merge rapid change bursts into a single reload batch. @@ -94,38 +93,101 @@ public FileWatcher(TimeSpan debounceWindow, Func? clock = null, public IReadOnlySet PendingChanges => new HashSet(_pending.Keys, StringComparer.OrdinalIgnoreCase); /// - /// Begins monitoring the given workspace root. Calling this while already watching a (possibly - /// different) root retargets monitoring to instead of throwing: the - /// previous is disposed and any pending change state accumulated - /// against the previous root is discarded, so no stale path can leak into the newly watched root's - /// pending set. + /// Begins monitoring the given source. Calling this again for a source whose id is already watched + /// retargets that one source instead of throwing: its previous is + /// disposed and replaced, without disturbing any other currently watched source or its pending change + /// state. A source is watched recursively; a + /// source is watched non-recursively on its containing directory, + /// filtered to that file's name. /// - /// Folder to observe. - /// Thrown when is null or whitespace. - /// Thrown when does not exist. - public void StartWatching(string rootPath) + /// Source to observe. + /// Thrown when is null. + /// + /// Thrown when the source's folder path (or, for a file source, its containing directory) does not exist. + /// + public void WatchSource(WorkspaceSource source) { - ArgumentException.ThrowIfNullOrWhiteSpace(rootPath); - if (!Directory.Exists(rootPath)) + ArgumentNullException.ThrowIfNull(source); + + if (_watchersBySourceId.Remove(source.Id, out var previous)) { - throw new DirectoryNotFoundException($"Workspace root folder was not found: {rootPath}"); + previous.Dispose(); } - _watcher?.Dispose(); - _watcher = null; - _pending.Clear(); + var watcher = source.Kind == WorkspaceSourceKind.Folder + ? CreateFolderWatcher(source.Path) + : CreateFileWatcher(source.Path); + + watcher.Changed += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); + watcher.Created += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); + watcher.Deleted += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); + watcher.Renamed += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); + watcher.EnableRaisingEvents = true; + + _watchersBySourceId[source.Id] = watcher; + } + + /// + /// Stops monitoring the given source, if it is currently watched. + /// + /// + /// Deliberately does not clear : pending changes contributed by other still-watched + /// sources must survive an unrelated source being removed, since each source's watcher is now independent + /// rather than one global watcher covering the whole workspace. + /// + /// Identifier of the source to stop watching. + /// when a watcher was found and removed; otherwise . + public bool UnwatchSource(string sourceId) + { + if (!_watchersBySourceId.Remove(sourceId, out var watcher)) + { + return false; + } + + watcher.Dispose(); + return true; + } + + /// + /// Creates a recursive watcher covering an entire folder. + /// + /// Folder to observe. + /// A configured, not-yet-enabled watcher. + /// Thrown when does not exist. + private static FileSystemWatcher CreateFolderWatcher(string folderPath) + { + if (!Directory.Exists(folderPath)) + { + throw new DirectoryNotFoundException($"Workspace folder was not found: {folderPath}"); + } - WatchedRootPath = Path.GetFullPath(rootPath); - _watcher = new FileSystemWatcher(WatchedRootPath) + return new FileSystemWatcher(folderPath) { IncludeSubdirectories = true, NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName | NotifyFilters.DirectoryName, }; - _watcher.Changed += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); - _watcher.Created += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); - _watcher.Deleted += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); - _watcher.Renamed += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); - _watcher.EnableRaisingEvents = true; + } + + /// + /// Creates a non-recursive watcher scoped to a single file via its containing directory and a name filter. + /// + /// File to observe. + /// A configured, not-yet-enabled watcher. + /// Thrown when the file's containing directory does not exist. + private static FileSystemWatcher CreateFileWatcher(string filePath) + { + var directory = Path.GetDirectoryName(filePath); + if (string.IsNullOrEmpty(directory) || !Directory.Exists(directory)) + { + throw new DirectoryNotFoundException($"Containing folder for workspace file was not found: {filePath}"); + } + + return new FileSystemWatcher(directory) + { + IncludeSubdirectories = false, + Filter = Path.GetFileName(filePath), + NotifyFilter = NotifyFilters.LastWrite | NotifyFilters.FileName | NotifyFilters.DirectoryName, + }; } /// @@ -133,13 +195,13 @@ public void StartWatching(string rootPath) /// /// File or folder path reported by the platform. /// Thrown when is null or whitespace. - /// Thrown when monitoring has not started. + /// Thrown when no source is currently watched. public void QueueChange(string path) { ArgumentException.ThrowIfNullOrWhiteSpace(path); - if (WatchedRootPath is null) + if (_watchersBySourceId.Count == 0) { - throw new InvalidOperationException("StartWatching must be called before changes can be queued."); + throw new InvalidOperationException("WatchSource must be called before changes can be queued."); } // Overwriting the timestamp for an already-pending path is what collapses bursty duplicate @@ -155,12 +217,12 @@ public void QueueChange(string path) /// returned; paths still within their debounce window remain pending for a later flush. /// /// Normalized paths requiring reload. - /// Thrown when monitoring has not started. + /// Thrown when no source is currently watched. public IReadOnlyList FlushPendingChanges() { - if (WatchedRootPath is null) + if (_watchersBySourceId.Count == 0) { - throw new InvalidOperationException("StartWatching must be called before changes can be flushed."); + throw new InvalidOperationException("WatchSource must be called before changes can be flushed."); } var now = _clock(); @@ -180,7 +242,11 @@ public IReadOnlyList FlushPendingChanges() /// public void Dispose() { - _watcher?.Dispose(); - _watcher = null; + foreach (var watcher in _watchersBySourceId.Values) + { + watcher.Dispose(); + } + + _watchersBySourceId.Clear(); } } diff --git a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceModel.cs b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceModel.cs index 5dd4d9a..564bc7f 100644 --- a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceModel.cs +++ b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceModel.cs @@ -1,4 +1,3 @@ -using DemaConsulting.SysML2Tools.Io; using DemaConsulting.SysML2Tools.Parser; using DemaConsulting.SysML2Tools.Semantic; using DemaConsulting.SysML2Tools.Stdlib; @@ -16,33 +15,17 @@ public sealed record WorkspaceFileState( IReadOnlyList Diagnostics, DateTimeOffset LoadedUtc); -/// -/// Options controlling how discovers and loads workspace files. -/// -/// Glob patterns (relative to the workspace root) selecting candidate model files. -/// -/// Extensions considered when a glob pattern's final segment is a bare wildcard. Ignored by patterns -/// that already name an extension (for example **/*.sysml). -/// -public sealed record WorkspaceLoadOptions(IReadOnlyList GlobPatterns, IReadOnlyList FileExtensions) -{ - /// - /// Default options matching the SysML2Tools CLI's own default: every .sysml file anywhere - /// under the workspace root. - /// - public static WorkspaceLoadOptions Default { get; } = new(["**/*.sysml"], []); -} - /// /// Immutable snapshot of a loaded workspace at a point in time. /// -/// Absolute workspace root folder the snapshot was loaded from. +/// Ordered file/folder sources the snapshot was resolved from. Empty for a first-class, +/// zero-source workspace. /// Discovered files that were combined into the workspace. /// Semantic workspace produced by SysML2Tools for the discovered files. /// All parser and semantic diagnostics produced by the load. /// Opaque token that changes every time a new snapshot is published. public sealed record WorkspaceSnapshot( - string RootPath, + IReadOnlyList Sources, IReadOnlyList Files, SysmlWorkspace Workspace, IReadOnlyList Diagnostics, @@ -62,7 +45,9 @@ public sealed record WorkspaceSnapshot( /// dependency edges. therefore performs a full workspace re-load rather than /// a dependency-scoped partial reload; per-file state entries whose diagnostics are unchanged after a reload /// keep their original value so callers can still tell which -/// files were actually affected by the last reload. +/// files were actually affected by the last reload. File discovery itself is no longer this unit's concern: +/// resolves the caller's file/folder sources into a merged file list, and +/// only ever loads the files it is handed. /// public sealed class WorkspaceModel { @@ -77,50 +62,49 @@ public sealed class WorkspaceModel private SysmlWorkspace? _workspace; /// - /// Absolute path to the workspace folder currently loaded, or before the first - /// load. + /// Resolution used by the most recent load, defaulting to an empty resolution so + /// is well-defined even before the first + /// call. /// - public string? RootPath { get; private set; } + private WorkspaceSourceResolution _resolution = new( + [], + new Dictionary(StringComparer.OrdinalIgnoreCase), + new Dictionary>(StringComparer.OrdinalIgnoreCase)); /// - /// Maps normalized file paths to the latest known parse result and file metadata. + /// Ordered file/folder sources currently loaded, or an empty list before the first load. /// - public IReadOnlyDictionary Files => _files; + public IReadOnlyList Sources { get; private set; } = []; /// - /// Glob patterns, standard library inclusion, and reload behavior that remain consistent for the life of - /// the loaded workspace. + /// Maps normalized file paths to the latest known parse result and file metadata. /// - public WorkspaceLoadOptions LoadOptions { get; private set; } = WorkspaceLoadOptions.Default; + public IReadOnlyDictionary Files => _files; /// - /// Creates a workspace snapshot from a root folder. + /// Loads a workspace snapshot from an already-resolved set of sources. /// /// - /// Discovers candidate files via , loads the SysML standard library - /// symbols needed by the parser pipeline, parses and resolves imports across the discovered file set via - /// , and records diagnostics per file before publishing the initial - /// snapshot. + /// Loads the SysML standard library symbols needed by the parser pipeline, parses and resolves imports + /// across 's merged file set via , and records + /// diagnostics per file before publishing the snapshot. A zero-source, zero-file resolution is a + /// first-class, valid input: called with an empty file list does + /// not throw and produces a valid, diagnostic-free, standard-library-only workspace. /// - /// Folder to scan for .sysml content. - /// Discovery and load options. Defaults to . - /// Normalized state for the full discovered workspace. - /// Thrown when is null or whitespace. - /// Thrown when does not exist. - public async Task LoadWorkspaceAsync(string rootPath, WorkspaceLoadOptions? options = null) + /// Ordered file/folder sources the resolution was computed from. + /// Already-resolved merged file set, typically from . + /// Normalized state for the resolved workspace. + /// Thrown when or is null. + public async Task LoadWorkspaceAsync(IReadOnlyList sources, WorkspaceSourceResolution resolution) { - ArgumentException.ThrowIfNullOrWhiteSpace(rootPath); - if (!Directory.Exists(rootPath)) - { - throw new DirectoryNotFoundException($"Workspace root folder was not found: {rootPath}"); - } + ArgumentNullException.ThrowIfNull(sources); + ArgumentNullException.ThrowIfNull(resolution); - // Normalize the root path once so all subsequent file-path comparisons are consistent - RootPath = Path.GetFullPath(rootPath); - LoadOptions = options ?? WorkspaceLoadOptions.Default; + Sources = sources; + _resolution = resolution; _files.Clear(); - var snapshot = await LoadInternalAsync(RootPath, LoadOptions).ConfigureAwait(false); + var snapshot = await LoadInternalAsync(sources, resolution.MergedFiles).ConfigureAwait(false); _workspace = snapshot.Workspace; return snapshot; } @@ -131,22 +115,33 @@ public async Task LoadWorkspaceAsync(string rootPath, Workspa /// /// See the remarks on for why this recomputes the whole workspace rather /// than a dependency-scoped subset: the real semantic model does not expose per-file import edges, so a - /// coherent reload requires reprocessing the full discovered file set. Only file entries whose - /// diagnostics actually changed receive a fresh . + /// coherent reload requires reprocessing the full resolved file set. Only file entries whose diagnostics + /// actually changed receive a fresh . Recomputes against the + /// current resolution even if it is empty (0 sources, 0 files) - this is a valid, non-throwing state. + /// When is supplied, it replaces the stored resolution before + /// recomputation - needed so a file created or deleted externally under a still-registered folder source + /// (as opposed to a source being explicitly added/removed) is picked up: an external file-system change + /// does not go through , so without a fresh resolution here, this method + /// would keep recomputing against the file list captured at the last explicit source-set mutation. When + /// omitted, the previously stored resolution is reused unchanged. /// /// Normalized files affected by an external change. + /// + /// Freshly recomputed resolution to adopt before reloading, or to keep reloading + /// against the resolution from the most recent call. + /// /// Updated workspace state after recomputation. /// Thrown when is null. - /// Thrown when no workspace has been loaded yet. - public async Task ReloadFilesAsync(IReadOnlyList changedPaths) + public async Task ReloadFilesAsync(IReadOnlyList changedPaths, WorkspaceSourceResolution? updatedResolution = null) { ArgumentNullException.ThrowIfNull(changedPaths); - if (RootPath is null) + + if (updatedResolution is not null) { - throw new InvalidOperationException("A workspace must be loaded before it can be reloaded."); + _resolution = updatedResolution; } - var snapshot = await LoadInternalAsync(RootPath, LoadOptions).ConfigureAwait(false); + var snapshot = await LoadInternalAsync(Sources, _resolution.MergedFiles).ConfigureAwait(false); _workspace = snapshot.Workspace; return snapshot; } @@ -167,17 +162,14 @@ public SysmlWorkspace GetSemanticWorkspace() } /// - /// Discovers, parses, and semantically resolves the workspace files rooted at , - /// then merges the resulting per-file diagnostics into . + /// Parses and semantically resolves , then merges the resulting + /// per-file diagnostics into . /// - /// Absolute workspace root folder. - /// Discovery and load options to apply. + /// Ordered file/folder sources the snapshot is published for. + /// Already-resolved, merged file set to load. /// The freshly computed workspace snapshot. - private async Task LoadInternalAsync(string rootPath, WorkspaceLoadOptions options) + private async Task LoadInternalAsync(IReadOnlyList sources, IReadOnlyList discoveredFiles) { - // Mirror the SysML2Tools CLI's own multi-file discovery so imports across files resolve the same way - var discoveredFiles = GlobFileCollector.Collect(options.GlobPatterns, options.FileExtensions, rootPath); - // The standard library symbol table is required so references into the SysML standard packages resolve var (symbolTable, _) = StdlibProvider.GetSymbolTable(); @@ -209,6 +201,6 @@ private async Task LoadInternalAsync(string rootPath, Workspa _files[path] = state; } - return new WorkspaceSnapshot(rootPath, discoveredFiles, loadResult.Workspace!, loadResult.Diagnostics, Guid.NewGuid().ToString("N")); + return new WorkspaceSnapshot(sources, discoveredFiles, loadResult.Workspace!, loadResult.Diagnostics, Guid.NewGuid().ToString("N")); } } diff --git a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs new file mode 100644 index 0000000..39b57c1 --- /dev/null +++ b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs @@ -0,0 +1,195 @@ +using DemaConsulting.SysML2Tools.Io; + +namespace DemaConsulting.SysML2Workbench.WorkspaceSubsystem; + +/// +/// Distinguishes whether a refers to a single file or a folder scanned for +/// matching model files. +/// +public enum WorkspaceSourceKind +{ + /// A single, explicitly added model file. + File, + + /// A folder scanned for model files using . + Folder, +} + +/// +/// Options controlling how discovers model files under a +/// source. +/// +/// Glob patterns (relative to the folder source) selecting candidate model files. +/// +/// Extensions considered when a glob pattern's final segment is a bare wildcard. Ignored by patterns +/// that already name an extension (for example **/*.sysml). +/// +public sealed record WorkspaceLoadOptions(IReadOnlyList GlobPatterns, IReadOnlyList FileExtensions) +{ + /// + /// Default options matching the SysML2Tools CLI's own default: every .sysml file anywhere + /// under the folder source. Applied to every source - folder + /// discovery is not individually configurable per source. + /// + public static WorkspaceLoadOptions Default { get; } = new(["**/*.sysml"], []); +} + +/// +/// One user-added workspace input: either a single file or a folder scanned for model files. +/// +/// Stable identifier that survives reorder/refresh; used as the file-watcher key and UI tree node key. +/// Whether this source is a single file or a scanned folder. +/// Absolute, normalized path (see ). +public sealed record WorkspaceSource(string Id, WorkspaceSourceKind Kind, string Path); + +/// +/// The result of resolving every source in a into a single, deduplicated +/// file list ready for . +/// +/// Deduped union of every file discovered across all sources, in registration order. +/// +/// Maps each merged file to the source that owns it. When the same file is reachable through more than one +/// source (for example a file explicitly added and also discovered under an overlapping folder), the +/// first-registered source wins attribution; this is a display-only tie-break and does not affect +/// , which contains the file exactly once regardless. +/// +/// +/// Maps each source id to the files it contributed: a source maps +/// to every file discovered under it, and a source maps to a +/// singleton list containing itself. Intended for tree-view consumers that need per-source grouping. +/// +public sealed record WorkspaceSourceResolution( + IReadOnlyList MergedFiles, + IReadOnlyDictionary FileToSourceId, + IReadOnlyDictionary> SourceIdToFiles); + +/// +/// WorkspaceSourceSet maintains the ordered set of file and folder sources a user has added to the workspace, +/// and resolves them into the merged, deduplicated file list consumed by . +/// +public sealed class WorkspaceSourceSet +{ + /// + /// Ordered, mutable backing list for . + /// + private readonly List _sources = []; + + /// + /// Every source currently added, in the order they were registered. + /// + public IReadOnlyList Sources => _sources; + + /// + /// Adds a single file source, or returns the existing source if the same normalized path is already + /// registered as a source. + /// + /// File path to add. + /// The newly created or already-existing source. + /// Thrown when is null or whitespace. + public WorkspaceSource AddFile(string path) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); + + var normalized = Path.GetFullPath(path); + var existing = _sources.FirstOrDefault(s => + s.Kind == WorkspaceSourceKind.File && string.Equals(s.Path, normalized, StringComparison.OrdinalIgnoreCase)); + if (existing is not null) + { + return existing; + } + + var source = new WorkspaceSource(Guid.NewGuid().ToString("N"), WorkspaceSourceKind.File, normalized); + _sources.Add(source); + return source; + } + + /// + /// Adds a folder source, or returns the existing source if the same normalized path is already registered + /// as a source. + /// + /// Folder path to add. + /// The newly created or already-existing source. + /// Thrown when is null or whitespace. + /// Thrown when does not exist. + public WorkspaceSource AddFolder(string path) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); + if (!Directory.Exists(path)) + { + throw new DirectoryNotFoundException($"Workspace folder was not found: {path}"); + } + + var normalized = Path.GetFullPath(path); + var existing = _sources.FirstOrDefault(s => + s.Kind == WorkspaceSourceKind.Folder && string.Equals(s.Path, normalized, StringComparison.OrdinalIgnoreCase)); + if (existing is not null) + { + return existing; + } + + var source = new WorkspaceSource(Guid.NewGuid().ToString("N"), WorkspaceSourceKind.Folder, normalized); + _sources.Add(source); + return source; + } + + /// + /// Removes the source with the given identifier, if one is registered. + /// + /// Identifier of the source to remove. + /// when a matching source was found and removed; otherwise . + public bool RemoveSource(string sourceId) + { + var index = _sources.FindIndex(s => s.Id == sourceId); + if (index < 0) + { + return false; + } + + _sources.RemoveAt(index); + return true; + } + + /// + /// Resolves every registered source into a single, deduplicated file list plus per-file and per-source + /// attribution. + /// + /// + /// Every source is scanned with + /// . Overlapping files (a file explicitly added that also + /// falls under a folder source, or two overlapping folder sources) are silently deduplicated: the + /// first-registered source to reach a given normalized path wins attribution in + /// . Zero sources resolves to an empty result with + /// no error. + /// + /// The merged file list and its source attribution. + public WorkspaceSourceResolution Resolve() + { + var mergedFiles = new List(); + var fileToSourceId = new Dictionary(StringComparer.OrdinalIgnoreCase); + var sourceIdToFiles = new Dictionary>(StringComparer.OrdinalIgnoreCase); + + foreach (var source in _sources) + { + var options = WorkspaceLoadOptions.Default; + IReadOnlyList sourceFiles = source.Kind == WorkspaceSourceKind.Folder + ? GlobFileCollector.Collect(options.GlobPatterns, options.FileExtensions, source.Path) + : [source.Path]; + + sourceIdToFiles[source.Id] = sourceFiles; + + foreach (var file in sourceFiles) + { + if (fileToSourceId.ContainsKey(file)) + { + // Already attributed to an earlier-registered source - dedupe silently, first source wins. + continue; + } + + fileToSourceId[file] = source.Id; + mergedFiles.Add(file); + } + } + + return new WorkspaceSourceResolution(mergedFiles, fileToSourceId, sourceIdToFiles); + } +} diff --git a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs index c9a7fd3..f690472 100644 --- a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs +++ b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs @@ -21,6 +21,16 @@ public void Dispose() } } + private static WorkspaceSource FolderSource(string path) + { + return new WorkspaceSource(Guid.NewGuid().ToString("N"), WorkspaceSourceKind.Folder, Path.GetFullPath(path)); + } + + private static WorkspaceSource FileSource(string path) + { + return new WorkspaceSource(Guid.NewGuid().ToString("N"), WorkspaceSourceKind.File, Path.GetFullPath(path)); + } + /// /// Validates that an externally reported file change is recorded as a pending path and is returned by a /// flush once its debounce window has elapsed. @@ -31,7 +41,7 @@ public void ExternalFileChange_RaisesAffectedPathEvent() // Arrange: a watcher with a fake clock so the debounce window can be advanced deterministically var now = DateTimeOffset.UtcNow; var watcher = new FileWatcher(TimeSpan.FromMilliseconds(50), () => now); - watcher.StartWatching(_tempRoot); + watcher.WatchSource(FolderSource(_tempRoot)); var changedPath = Path.Combine(_tempRoot, "Model.sysml"); // Act: simulate an external change notification, then advance the clock past the debounce window @@ -55,7 +65,7 @@ public void NotificationBurst_CoalescesIntoSingleReloadTrigger() // Arrange: a watcher with a fake clock var now = DateTimeOffset.UtcNow; var watcher = new FileWatcher(TimeSpan.FromMilliseconds(50), () => now); - watcher.StartWatching(_tempRoot); + watcher.WatchSource(FolderSource(_tempRoot)); var changedPath = Path.Combine(_tempRoot, "Model.sysml"); // Act: raise a burst of notifications for the same path in quick succession @@ -84,7 +94,7 @@ public void FlushPendingChanges_WithinDebounceWindow_RetainsPathForLaterFlush() // Arrange: a watcher with a fake clock and a change just registered var now = DateTimeOffset.UtcNow; var watcher = new FileWatcher(TimeSpan.FromSeconds(1), () => now); - watcher.StartWatching(_tempRoot); + watcher.WatchSource(FolderSource(_tempRoot)); var changedPath = Path.Combine(_tempRoot, "Model.sysml"); watcher.QueueChange(changedPath); @@ -97,13 +107,13 @@ public void FlushPendingChanges_WithinDebounceWindow_RetainsPathForLaterFlush() } /// - /// Validates that queuing a change before monitoring has started is rejected rather than silently + /// Validates that queuing a change before any source is watched is rejected rather than silently /// accepted. /// [Fact] - public void QueueChange_BeforeStartWatching_ThrowsInvalidOperationException() + public void QueueChange_BeforeWatchSource_ThrowsInvalidOperationException() { - // Arrange: a watcher that has never started monitoring + // Arrange: a watcher that has never started monitoring any source var watcher = new FileWatcher(TimeSpan.FromMilliseconds(50)); // Act / Assert: queuing throws instead of silently doing nothing @@ -111,33 +121,119 @@ public void QueueChange_BeforeStartWatching_ThrowsInvalidOperationException() } /// - /// Validates that calling a second time with a different root - /// retargets monitoring instead of throwing, and discards any pending state accumulated against the - /// previously watched root so it cannot leak into the newly watched root's pending set. + /// Validates that tracks each distinct source id independently, so + /// watching two different sources results in both ids being reported as watched. /// [Fact] - public void StartWatching_CalledTwice_RetargetsToNewRootAndDiscardsPendingChanges() + public void WatchSource_TwoDistinctSources_BothTrackedIndependently() { - // Arrange: watch root A and queue a pending change against it - var rootA = _tempRoot; - var rootB = Directory.CreateTempSubdirectory("sysml2workbench-tests-").FullName; + // Arrange + var folderA = FolderSource(_tempRoot); + var otherRoot = Directory.CreateTempSubdirectory("sysml2workbench-tests-").FullName; try { + var folderB = FolderSource(otherRoot); + var watcher = new FileWatcher(TimeSpan.FromMilliseconds(50)); + + // Act + watcher.WatchSource(folderA); + watcher.WatchSource(folderB); + + // Assert + Assert.Equal(2, watcher.WatchedSourceIds.Count); + Assert.Contains(folderA.Id, watcher.WatchedSourceIds); + Assert.Contains(folderB.Id, watcher.WatchedSourceIds); + } + finally + { + Directory.Delete(otherRoot, recursive: true); + } + } + + /// + /// Validates that stops monitoring only the given source, + /// leaving other currently watched sources - and their pending changes - untouched: a change queued + /// under folder A before A is unwatched must not be discarded just because A stopped being watched, and + /// folder B's own pending state must be unaffected by the operation. + /// + [Fact] + public void UnwatchSource_RemovesOnlyThatWatcher_OthersContinueReporting() + { + // Arrange: two watched folder sources, each with a pending change queued against it + var folderA = FolderSource(_tempRoot); + var otherRoot = Directory.CreateTempSubdirectory("sysml2workbench-tests-").FullName; + try + { + var folderB = FolderSource(otherRoot); var now = DateTimeOffset.UtcNow; var watcher = new FileWatcher(TimeSpan.FromSeconds(1), () => now); - watcher.StartWatching(rootA); - var changedPathUnderA = Path.Combine(rootA, "Model.sysml"); - watcher.QueueChange(changedPathUnderA); - Assert.Contains(changedPathUnderA, watcher.PendingChanges); + watcher.WatchSource(folderA); + watcher.WatchSource(folderB); + + var pathUnderA = Path.Combine(folderA.Path, "A.sysml"); + var pathUnderB = Path.Combine(folderB.Path, "B.sysml"); + watcher.QueueChange(pathUnderA); + watcher.QueueChange(pathUnderB); + + // Act: unwatch only source A + var removed = watcher.UnwatchSource(folderA.Id); + + // Assert: A is no longer watched, B still is, and neither pending change was discarded + Assert.True(removed); + Assert.DoesNotContain(folderA.Id, watcher.WatchedSourceIds); + Assert.Contains(folderB.Id, watcher.WatchedSourceIds); + Assert.Contains(pathUnderA, watcher.PendingChanges); + Assert.Contains(pathUnderB, watcher.PendingChanges); + } + finally + { + Directory.Delete(otherRoot, recursive: true); + } + } + + /// + /// Validates that returns for a source + /// id that is not currently watched, rather than throwing. + /// + [Fact] + public void UnwatchSource_UnknownSourceId_ReturnsFalse() + { + // Arrange + var watcher = new FileWatcher(TimeSpan.FromMilliseconds(50)); + + // Act + var removed = watcher.UnwatchSource("unknown-source-id"); + + // Assert + Assert.False(removed); + } + + /// + /// Validates that calling a second time for the same source id + /// retargets that one source instead of throwing, and does not disturb any other watched source. + /// + [Fact] + public void WatchSource_CalledTwiceForSameSourceId_RetargetsWithoutThrowing() + { + // Arrange: watch a folder source, then build a second WorkspaceSource with the same id but a different + // path, simulating a source whose path was retargeted while its id stayed stable. + var rootA = _tempRoot; + var rootB = Directory.CreateTempSubdirectory("sysml2workbench-tests-").FullName; + try + { + var sourceId = Guid.NewGuid().ToString("N"); + var sourceA = new WorkspaceSource(sourceId, WorkspaceSourceKind.Folder, Path.GetFullPath(rootA)); + var sourceB = new WorkspaceSource(sourceId, WorkspaceSourceKind.Folder, Path.GetFullPath(rootB)); + var watcher = new FileWatcher(TimeSpan.FromSeconds(1)); + watcher.WatchSource(sourceA); - // Act: retarget to root B - must not throw, despite root A never being explicitly stopped - var exception = Record.Exception(() => watcher.StartWatching(rootB)); + // Act: retarget - must not throw, despite the source never being explicitly unwatched + var exception = Record.Exception(() => watcher.WatchSource(sourceB)); - // Assert: no exception, watched root updated to B, and A's pending change did not leak into B + // Assert: no exception, and the source id is still reported as watched exactly once Assert.Null(exception); - Assert.Equal(Path.GetFullPath(rootB), watcher.WatchedRootPath); - Assert.Empty(watcher.PendingChanges); - Assert.DoesNotContain(changedPathUnderA, watcher.PendingChanges); + Assert.Single(watcher.WatchedSourceIds); + Assert.Contains(sourceId, watcher.WatchedSourceIds); } finally { @@ -146,29 +242,28 @@ public void StartWatching_CalledTwice_RetargetsToNewRootAndDiscardsPendingChange } /// - /// Validates that after retargeting from root A to root B, a real operating-system file-system event - /// raised under B is actually detected, proving the underlying was truly - /// rebuilt against the new root rather than merely having - /// relabeled. + /// Validates that a real operating-system file-system event raised under one watched folder source is + /// detected as a pending change, while a change under a second, independently watched folder source does + /// not get attributed to the first - proving each source genuinely has its own isolated watch scope. /// /// - /// Uses the real system clock and a real (no fake clock, unlike this - /// file's other tests), and polls with a bounded timeout since OS file-system notifications are - /// delivered asynchronously and are not deterministic in timing. + /// Uses the real system clock and real instances (no fake + /// clock, unlike this file's other tests), and polls with a bounded timeout since OS file-system + /// notifications are delivered asynchronously and are not deterministic in timing. /// [Fact] - public async Task StartWatching_RetargetedRoot_DetectsRealFileChangeUnderNewRoot() + public async Task WatchSource_TwoFolders_ChangeUnderOneIsNotAttributedToTheOther() { - // Arrange: watch root A, then retarget to root B + // Arrange: watch two independent folder sources var rootA = _tempRoot; var rootB = Directory.CreateTempSubdirectory("sysml2workbench-tests-").FullName; try { var watcher = new FileWatcher(TimeSpan.FromMilliseconds(50)); - watcher.StartWatching(rootA); - watcher.StartWatching(rootB); + watcher.WatchSource(FolderSource(rootA)); + watcher.WatchSource(FolderSource(rootB)); - // Act: write a real file under the newly targeted root B + // Act: write a real file under root B only var changedPathUnderB = Path.Combine(rootB, "Model.sysml"); await File.WriteAllTextAsync(changedPathUnderB, "part def Vehicle;", TestContext.Current.CancellationToken); @@ -195,8 +290,10 @@ public async Task StartWatching_RetargetedRoot_DetectsRealFileChangeUnderNewRoot } } - // Assert: the real OS-level change under the new root B was detected - Assert.True(detected, $"Expected '{changedPathUnderB}' to be reported as a pending change under the retargeted root."); + // Assert: the real OS-level change under B was detected, and nothing under root A was ever written, + // so no path under A can appear as a false-positive pending change. + Assert.True(detected, $"Expected '{changedPathUnderB}' to be reported as a pending change under root B."); + Assert.DoesNotContain(watcher.PendingChanges, path => path.StartsWith(rootA, StringComparison.OrdinalIgnoreCase)); } finally { diff --git a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceModelTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceModelTests.cs index 7085baa..a174b1b 100644 --- a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceModelTests.cs +++ b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceModelTests.cs @@ -32,25 +32,39 @@ private static Task WriteFileAsync(string path, string content) return File.WriteAllTextAsync(path, content, TestContext.Current.CancellationToken); } + /// + /// Loads a single folder source into a fresh and returns both the + /// sources and the resolution, for tests that only care about the resulting + /// state rather than the source-set mechanics themselves. + /// + private static (IReadOnlyList Sources, WorkspaceSourceResolution Resolution) ResolveFolder(string folderPath) + { + var sourceSet = new WorkspaceSourceSet(); + sourceSet.AddFolder(folderPath); + return (sourceSet.Sources, sourceSet.Resolve()); + } + /// /// Validates that loading a workspace folder builds a tracked file tree covering every discovered file. /// [Fact] - public async Task OpenWorkspace_BuildsTrackedFileTree() + public async Task LoadWorkspaceAsync_BuildsTrackedFileTree() { // Arrange: a workspace folder containing a single valid SysML file var filePath = Path.Combine(_tempRoot, "Sample.sysml"); await WriteFileAsync(filePath, "package Sample {\n part def Widget;\n}\n"); var model = new WorkspaceModel(); + var (sources, resolution) = ResolveFolder(_tempRoot); // Act: load the workspace - var snapshot = await model.LoadWorkspaceAsync(_tempRoot); + var snapshot = await model.LoadWorkspaceAsync(sources, resolution); - // Assert: the tracked file tree reflects the discovered file and the workspace root - Assert.Equal(Path.GetFullPath(_tempRoot), model.RootPath); + // Assert: the tracked file tree reflects the discovered file and the resolved sources + Assert.Single(model.Sources); Assert.Single(model.Files); Assert.True(model.Files.ContainsKey(filePath)); Assert.Single(snapshot.Files); + Assert.Single(snapshot.Sources); } /// @@ -58,7 +72,7 @@ public async Task OpenWorkspace_BuildsTrackedFileTree() /// resolution across those discovered files. /// [Fact] - public async Task ResolveInputs_FindsDiscoveredAndImportedFiles() + public async Task LoadWorkspaceAsync_FindsDiscoveredAndImportedFiles() { // Arrange: two files where the second imports a definition declared in the first await WriteFileAsync( @@ -68,9 +82,10 @@ await WriteFileAsync( Path.Combine(_tempRoot, "B.sysml"), "package PackageB {\n import PackageA::*;\n part myWidget : Widget;\n}\n"); var model = new WorkspaceModel(); + var (sources, resolution) = ResolveFolder(_tempRoot); // Act: load the workspace - var snapshot = await model.LoadWorkspaceAsync(_tempRoot); + var snapshot = await model.LoadWorkspaceAsync(sources, resolution); // Assert: both files were discovered and the cross-file import resolved without errors Assert.Equal(2, snapshot.Files.Count); @@ -92,7 +107,8 @@ public async Task ReloadFile_UpdatesOnlyAffectedFileState() await WriteFileAsync(unaffectedPath, "package Unaffected {\n part def Widget;\n}\n"); await WriteFileAsync(affectedPath, "package Affected {\n part def Gadget;\n}\n"); var model = new WorkspaceModel(); - await model.LoadWorkspaceAsync(_tempRoot); + var (sources, resolution) = ResolveFolder(_tempRoot); + await model.LoadWorkspaceAsync(sources, resolution); var unaffectedBefore = model.Files[unaffectedPath]; var affectedBefore = model.Files[affectedPath]; @@ -108,18 +124,51 @@ public async Task ReloadFile_UpdatesOnlyAffectedFileState() } /// - /// Validates that loading from a folder that does not exist reports a clear, propagated failure rather - /// than an empty workspace. + /// Validates that a zero-source, zero-file resolution is first-class: it produces a valid, + /// diagnostic-free, standard-library-only snapshot rather than throwing. + /// + [Fact] + public async Task LoadWorkspaceAsync_EmptyResolution_ProducesValidStdlibOnlySnapshot() + { + // Arrange: an empty source set, resolved to zero sources and zero files + var model = new WorkspaceModel(); + var sourceSet = new WorkspaceSourceSet(); + var resolution = sourceSet.Resolve(); + + // Act: load the empty resolution + var snapshot = await model.LoadWorkspaceAsync(sourceSet.Sources, resolution); + + // Assert: a valid, non-throwing, diagnostic-free snapshot with no sources and no files + Assert.Empty(snapshot.Sources); + Assert.Empty(snapshot.Files); + Assert.Empty(snapshot.Diagnostics); + Assert.NotNull(snapshot.Workspace); + } + + /// + /// Validates that reloading against a current resolution that has since become empty (0 sources, 0 + /// files) is a valid, non-throwing no-op recomputation rather than an error. /// [Fact] - public async Task LoadWorkspaceAsync_MissingRootFolder_ThrowsDirectoryNotFoundException() + public async Task ReloadFilesAsync_AfterResolutionBecomesEmpty_ProducesValidEmptySnapshot() { - // Arrange: a path that does not exist on disk - var missingPath = Path.Combine(_tempRoot, "does-not-exist"); + // Arrange: load a non-empty workspace, then reset it to an empty resolution (mirrors what + // MainWindowShell does after removing the last source). + var filePath = Path.Combine(_tempRoot, "Sample.sysml"); + await WriteFileAsync(filePath, "package Sample {\n part def Widget;\n}\n"); var model = new WorkspaceModel(); + var (sources, resolution) = ResolveFolder(_tempRoot); + await model.LoadWorkspaceAsync(sources, resolution); + + var emptySourceSet = new WorkspaceSourceSet(); + await model.LoadWorkspaceAsync(emptySourceSet.Sources, emptySourceSet.Resolve()); + + // Act: reload against the now-empty resolution + var snapshot = await model.ReloadFilesAsync([]); - // Act / Assert: loading throws instead of silently producing an empty workspace - await Assert.ThrowsAsync(() => model.LoadWorkspaceAsync(missingPath)); + // Assert: a valid, empty snapshot, and no lingering per-file state + Assert.Empty(snapshot.Files); + Assert.Empty(model.Files); } /// diff --git a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs new file mode 100644 index 0000000..5066111 --- /dev/null +++ b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs @@ -0,0 +1,229 @@ +using DemaConsulting.SysML2Workbench.WorkspaceSubsystem; + +namespace DemaConsulting.SysML2Workbench.Tests.WorkspaceSubsystem; + +/// +/// Unit tests for . +/// +public sealed class WorkspaceSourceSetTests : IDisposable +{ + /// + /// Temporary root folder created fresh for each test and removed on disposal. + /// + private readonly string _tempRoot = Directory.CreateTempSubdirectory("sysml2workbench-source-set-tests-").FullName; + + /// + public void Dispose() + { + if (Directory.Exists(_tempRoot)) + { + Directory.Delete(_tempRoot, recursive: true); + } + } + + private static Task WriteFileAsync(string path, string content) + { + return File.WriteAllTextAsync(path, content, TestContext.Current.CancellationToken); + } + + /// + /// Validates that a freshly constructed source set has no sources and resolves to an empty result with + /// no error. + /// + [Fact] + public void Resolve_ZeroSources_ReturnsEmptyResolution() + { + // Arrange + var sourceSet = new WorkspaceSourceSet(); + + // Act + var resolution = sourceSet.Resolve(); + + // Assert + Assert.Empty(sourceSet.Sources); + Assert.Empty(resolution.MergedFiles); + Assert.Empty(resolution.FileToSourceId); + Assert.Empty(resolution.SourceIdToFiles); + } + + /// + /// Validates that adding a file source is idempotent: adding the exact same path twice returns the same + /// source and does not create a duplicate entry. + /// + [Fact] + public async Task AddFile_SamePathTwice_ReturnsSameSourceAndDoesNotDuplicate() + { + // Arrange + var filePath = Path.Combine(_tempRoot, "Sample.sysml"); + await WriteFileAsync(filePath, "package Sample {\n part def Widget;\n}\n"); + var sourceSet = new WorkspaceSourceSet(); + + // Act + var first = sourceSet.AddFile(filePath); + var second = sourceSet.AddFile(filePath); + + // Assert + Assert.Equal(first.Id, second.Id); + Assert.Single(sourceSet.Sources); + } + + /// + /// Validates that adding a folder source is idempotent: adding the exact same path twice returns the same + /// source and does not create a duplicate entry. + /// + [Fact] + public void AddFolder_SamePathTwice_ReturnsSameSourceAndDoesNotDuplicate() + { + // Arrange + var sourceSet = new WorkspaceSourceSet(); + + // Act + var first = sourceSet.AddFolder(_tempRoot); + var second = sourceSet.AddFolder(_tempRoot); + + // Assert + Assert.Equal(first.Id, second.Id); + Assert.Single(sourceSet.Sources); + } + + /// + /// Validates that throws a clear, fail-fast error for a + /// folder that does not exist, rather than silently registering a source that will always resolve to + /// zero files. + /// + [Fact] + public void AddFolder_MissingFolder_ThrowsDirectoryNotFoundException() + { + // Arrange + var missingPath = Path.Combine(_tempRoot, "does-not-exist"); + var sourceSet = new WorkspaceSourceSet(); + + // Act / Assert + Assert.Throws(() => sourceSet.AddFolder(missingPath)); + } + + /// + /// Validates that removes a registered source and returns + /// , and returns for an unknown identifier. + /// + [Fact] + public void RemoveSource_RegisteredThenUnknownId_ReturnsTrueThenFalse() + { + // Arrange + var sourceSet = new WorkspaceSourceSet(); + var source = sourceSet.AddFolder(_tempRoot); + + // Act + var removed = sourceSet.RemoveSource(source.Id); + var removedAgain = sourceSet.RemoveSource(source.Id); + + // Assert + Assert.True(removed); + Assert.False(removedAgain); + Assert.Empty(sourceSet.Sources); + } + + /// + /// Validates that resolving a folder source discovers every .sysml file under it, recursively. + /// + [Fact] + public async Task Resolve_FolderSource_DiscoversAllSysmlFilesRecursively() + { + // Arrange + var nestedDir = Path.Combine(_tempRoot, "nested"); + Directory.CreateDirectory(nestedDir); + await WriteFileAsync(Path.Combine(_tempRoot, "Top.sysml"), "package Top {\n part def Widget;\n}\n"); + await WriteFileAsync(Path.Combine(nestedDir, "Nested.sysml"), "package Nested {\n part def Gadget;\n}\n"); + var sourceSet = new WorkspaceSourceSet(); + var folderSource = sourceSet.AddFolder(_tempRoot); + + // Act + var resolution = sourceSet.Resolve(); + + // Assert + Assert.Equal(2, resolution.MergedFiles.Count); + Assert.Equal(2, resolution.SourceIdToFiles[folderSource.Id].Count); + } + + /// + /// Validates that a file explicitly added and also discovered under an overlapping folder source is + /// deduplicated to a single entry in , and that the + /// first-registered source wins attribution in . + /// + [Fact] + public async Task Resolve_FileOverlappingFolder_DedupesAndFirstRegisteredSourceWinsAttribution() + { + // Arrange: the file source is registered first, then a folder source that also discovers that file + var filePath = Path.Combine(_tempRoot, "Sample.sysml"); + await WriteFileAsync(filePath, "package Sample {\n part def Widget;\n}\n"); + var sourceSet = new WorkspaceSourceSet(); + var fileSource = sourceSet.AddFile(filePath); + var folderSource = sourceSet.AddFolder(_tempRoot); + + // Act + var resolution = sourceSet.Resolve(); + + // Assert: deduped to one entry, attributed to the first-registered (file) source + Assert.Single(resolution.MergedFiles); + Assert.Equal(fileSource.Id, resolution.FileToSourceId[resolution.MergedFiles[0]]); + // The folder source's own per-source file list still independently lists the file it discovered. + Assert.Single(resolution.SourceIdToFiles[folderSource.Id]); + } + + /// + /// Validates that two overlapping folder sources (one nested inside the other) dedupe their shared files + /// to a single merged entry, attributed to the first-registered folder. + /// + [Fact] + public async Task Resolve_NestedFolderOverlap_DedupesAndFirstRegisteredSourceWinsAttribution() + { + // Arrange: an outer folder registered first, and an inner (nested) folder registered second, both + // discovering the same nested file. + var nestedDir = Path.Combine(_tempRoot, "nested"); + Directory.CreateDirectory(nestedDir); + var nestedFile = Path.Combine(nestedDir, "Nested.sysml"); + await WriteFileAsync(nestedFile, "package Nested {\n part def Gadget;\n}\n"); + var sourceSet = new WorkspaceSourceSet(); + var outerSource = sourceSet.AddFolder(_tempRoot); + sourceSet.AddFolder(nestedDir); + + // Act + var resolution = sourceSet.Resolve(); + + // Assert: the nested file appears once, attributed to the first-registered (outer) folder + Assert.Single(resolution.MergedFiles); + Assert.Equal(outerSource.Id, resolution.FileToSourceId[resolution.MergedFiles[0]]); + } + + /// + /// Validates that preserves registration order across mixed + /// file and folder additions. + /// + [Fact] + public async Task Sources_PreservesRegistrationOrder() + { + // Arrange + var filePath = Path.Combine(_tempRoot, "Sample.sysml"); + await WriteFileAsync(filePath, "package Sample {\n part def Widget;\n}\n"); + var otherDir = Directory.CreateTempSubdirectory("sysml2workbench-source-set-tests-other-").FullName; + try + { + var sourceSet = new WorkspaceSourceSet(); + + // Act + var folderSource = sourceSet.AddFolder(_tempRoot); + var fileSource = sourceSet.AddFile(filePath); + var secondFolderSource = sourceSet.AddFolder(otherDir); + + // Assert + Assert.Equal([folderSource, fileSource, secondFolderSource], sourceSet.Sources); + } + finally + { + if (Directory.Exists(otherDir)) + { + Directory.Delete(otherDir, recursive: true); + } + } + } +} diff --git a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystemTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystemTests.cs index 8bd9eac..654d562 100644 --- a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystemTests.cs +++ b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystemTests.cs @@ -35,7 +35,9 @@ await File.WriteAllTextAsync( var model = new WorkspaceModel(); // Act - var snapshot = await model.LoadWorkspaceAsync(_tempRoot); + var sourceSet = new WorkspaceSourceSet(); + sourceSet.AddFolder(_tempRoot); + var snapshot = await model.LoadWorkspaceAsync(sourceSet.Sources, sourceSet.Resolve()); // Assert: the file tree, semantic workspace, and diagnostics are all populated from the same load pass Assert.Single(snapshot.Files); @@ -57,11 +59,13 @@ await File.WriteAllTextAsync( "package Sample {\n part def Engine;\n}\n", TestContext.Current.CancellationToken); var model = new WorkspaceModel(); - await model.LoadWorkspaceAsync(_tempRoot); + var sourceSet = new WorkspaceSourceSet(); + sourceSet.AddFolder(_tempRoot); + await model.LoadWorkspaceAsync(sourceSet.Sources, sourceSet.Resolve()); var now = DateTimeOffset.UtcNow; var watcher = new FileWatcher(TimeSpan.FromMilliseconds(1), () => now); - watcher.StartWatching(_tempRoot); + watcher.WatchSource(sourceSet.Sources[0]); // Act: an external process adds a new file, the watcher observes it, and the debounce window elapses var newFilePath = Path.Combine(_tempRoot, "Extra.sysml"); @@ -69,7 +73,7 @@ await File.WriteAllTextAsync( watcher.QueueChange(newFilePath); now = now.AddSeconds(1); var changed = watcher.FlushPendingChanges(); - var refreshed = await model.ReloadFilesAsync(changed); + var refreshed = await model.ReloadFilesAsync(changed, sourceSet.Resolve()); // Assert: the refreshed workspace state now includes the new file Assert.Equal(2, refreshed.Files.Count); @@ -98,7 +102,9 @@ await File.WriteAllTextAsync( var model = new WorkspaceModel(); // Act - var snapshot = await model.LoadWorkspaceAsync(_tempRoot); + var sourceSet = new WorkspaceSourceSet(); + sourceSet.AddFolder(_tempRoot); + var snapshot = await model.LoadWorkspaceAsync(sourceSet.Sources, sourceSet.Resolve()); var aggregator = new DiagnosticsAggregator(); aggregator.ReplaceWorkspaceDiagnostics(snapshot.Diagnostics); var ordered = aggregator.RebuildAggregate(); From 7d7df769a76a51d99b5d39d5925caeb687194163 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 18:11:33 -0400 Subject: [PATCH 02/14] feat(shell): add multi-source Add/Remove APIs, Workspace panel, and empty-state UI MainWindowShell now owns a WorkspaceSourceSet and exposes AddFileSourceAsync/AddFolderSourceAsync/RemoveSourceAsync in place of OpenWorkspaceAsync. CurrentWorkspace is never null - the shell establishes a valid empty snapshot (0 sources) at construction - and every mutator diffs the previous/new watch set against FileWatcher, applies the new snapshot, and raises a new SourcesChanged event. RefreshFromExternalChangesAsync now re-resolves the source set before reloading so externally created/deleted files under a watched folder are actually picked up. SelectPredefinedView/ PreviewCustomView now guard on Sources.Count == 0 instead of a null check. Adds a new Workspace dock panel (WorkspacePanelToolViewModel/View) showing sources as a tree - folders expand to their files, files are leaves - with Add File/Add Folder/Remove commands and drag-and-drop of files/folders onto the panel or main window, funneled through the same shell APIs. Wires it into WorkbenchDockFactory (now a 4th required panel) and MainWindowView's File/View menus and picker/drop handlers. Adds empty-state messaging (Sources.Count == 0) to PredefinedViewsToolViewModel, CustomViewBuilderToolViewModel, and DiagnosticsToolViewModel/View, with the diagnostics panel wording distinctly for "workspace is empty" versus "no issues found". Updates every remaining caller of the old single-root OpenWorkspaceAsync/ RootPath API across the test suite, including the previously undocumented OtsSoftwareTests project (WorkbenchDockFactory's constructor gained a 4th required parameter). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../CustomViewBuilderToolView.axaml | 2 + .../CustomViewBuilderToolViewModel.cs | 8 +- .../DiagnosticsToolView.axaml | 1 + .../DiagnosticsToolViewModel.cs | 21 ++ .../AppShellSubsystem/MainWindowShell.cs | 209 ++++++++++--- .../AppShellSubsystem/MainWindowView.axaml | 6 +- .../AppShellSubsystem/MainWindowView.axaml.cs | 120 +++++++- .../PredefinedViewsToolView.axaml | 1 + .../PredefinedViewsToolViewModel.cs | 6 + .../AppShellSubsystem/WorkbenchDockFactory.cs | 21 +- .../WorkspacePanelToolView.axaml | 40 +++ .../WorkspacePanelToolView.axaml.cs | 125 ++++++++ .../WorkspacePanelToolViewModel.cs | 181 +++++++++++ .../AppShellSubsystem/MainWindowShellTests.cs | 121 ++++++-- .../WorkspacePanelToolViewModelTests.cs | 291 ++++++++++++++++++ .../AppShellSubsystemTests.cs | 6 +- .../LayoutInvokerTests.cs | 6 +- .../LayoutRenderingSubsystemTests.cs | 4 +- .../SysML2WorkbenchTests.cs | 12 +- .../ViewDefinitionModelTests.cs | 6 +- .../ViewBuilderSubsystemTests.cs | 4 +- .../ViewCatalogPresenterTests.cs | 10 +- .../ViewCatalogSubsystemTests.cs | 6 +- test/OtsSoftwareTests/AvaloniaTests.cs | 2 +- test/OtsSoftwareTests/DockTests.cs | 24 +- 25 files changed, 1124 insertions(+), 109 deletions(-) create mode 100644 src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml create mode 100644 src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml.cs create mode 100644 src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs create mode 100644 test/DemaConsulting.SysML2Workbench.Tests/AppShellSubsystem/WorkspacePanelToolViewModelTests.cs diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolView.axaml index 8f1b33a..f31c239 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolView.axaml +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolView.axaml @@ -7,6 +7,8 @@ + + diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolViewModel.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolViewModel.cs index 53f76b4..ccae729 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolViewModel.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/CustomViewBuilderToolViewModel.cs @@ -35,6 +35,9 @@ public partial class CustomViewBuilderToolViewModel : Dock.Model.Mvvm.Controls.T [ObservableProperty] private string? _statusMessage; + [ObservableProperty] + private bool _isWorkspaceEmpty; + /// /// Creates the custom-view builder tool view model. /// @@ -42,6 +45,8 @@ public partial class CustomViewBuilderToolViewModel : Dock.Model.Mvvm.Controls.T public CustomViewBuilderToolViewModel(MainWindowShell shell) { Shell = shell ?? throw new ArgumentNullException(nameof(shell)); + Shell.SourcesChanged += (_, _) => RefreshFromWorkspace(); + RefreshFromWorkspace(); } /// @@ -71,7 +76,7 @@ public CustomViewBuilderToolViewModel(MainWindowShell shell) /// public void RefreshFromWorkspace() { - AvailableExposeTargets = Shell.CurrentWorkspace is null + AvailableExposeTargets = Shell.CurrentWorkspace.Sources.Count == 0 ? [] : Shell.CurrentWorkspace.Workspace.Declarations .Where(kvp => !Shell.CurrentWorkspace.Workspace.StdlibNames.Contains(kvp.Key)) @@ -82,6 +87,7 @@ public void RefreshFromWorkspace() BuilderDefinition = new ViewDefinitionModel(); StatusMessage = null; + IsWorkspaceEmpty = Shell.CurrentWorkspace.Sources.Count == 0; BuilderReset?.Invoke(this, EventArgs.Empty); } } diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolView.axaml index 8192065..f88038c 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolView.axaml +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolView.axaml @@ -6,6 +6,7 @@ + diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolViewModel.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolViewModel.cs index f9bfd11..8575200 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolViewModel.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/DiagnosticsToolViewModel.cs @@ -14,6 +14,12 @@ public partial class DiagnosticsToolViewModel : Dock.Model.Mvvm.Controls.Tool [ObservableProperty] private IReadOnlyList _visibleDiagnostics = []; + [ObservableProperty] + private string? _emptyStateMessage; + + [ObservableProperty] + private bool _hasEmptyStateMessage; + /// /// Creates the diagnostics tool view model. /// @@ -21,13 +27,28 @@ public partial class DiagnosticsToolViewModel : Dock.Model.Mvvm.Controls.Tool public DiagnosticsToolViewModel(MainWindowShell shell) { _shell = shell ?? throw new ArgumentNullException(nameof(shell)); + _shell.SourcesChanged += (_, _) => RefreshFromWorkspace(); + RefreshFromWorkspace(); } /// /// Refreshes the diagnostics list from current shell state after a workspace open or reload. /// + /// + /// When there are zero diagnostics to show, distinguishes why: a zero-source workspace shows + /// "No diagnostics - workspace is empty", while a loaded workspace with no actual problems shows the + /// differently worded "No issues found" - these are deliberately phrased differently so a user cannot + /// mistake an empty workspace for a clean one. + /// public void RefreshFromWorkspace() { VisibleDiagnostics = _shell.Diagnostics.VisibleDiagnostics; + EmptyStateMessage = VisibleDiagnostics.Count switch + { + 0 when _shell.CurrentWorkspace.Sources.Count == 0 => "No diagnostics - workspace is empty", + 0 => "No issues found", + _ => null, + }; + HasEmptyStateMessage = EmptyStateMessage is not null; } } diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs index 3ffd5b4..cc4eacf 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs @@ -57,6 +57,20 @@ public sealed class MainWindowShell : IDisposable private readonly RollingFileLogger _logger; private readonly List _openTabs = []; + /// + /// Owns the ordered set of file/folder sources the user has added to the workspace. The shell is the + /// single owner of this instance; other consumers (for example ) + /// read source/attribution state only through the shell's own read-only surface. + /// + private readonly WorkspaceSourceSet _sourceSet = new(); + + /// + /// Identifiers of the sources currently registered with , tracked so mutators + /// can diff the previous and new source sets and call / + /// only for the sources that actually changed. + /// + private readonly HashSet _watchedSourceIds = []; + /// /// Canvas shown when no diagram tab is open, so always has a usable instance to /// return even before the first tab exists (or after the last tab closes). @@ -71,10 +85,20 @@ public sealed class MainWindowShell : IDisposable private int _customPreviewTabSequence; /// - /// Currently loaded workspace and its revision metadata, or before the first - /// workspace is opened. + /// Currently loaded workspace and its revision metadata. Never : the shell + /// eagerly computes and applies an empty (0-source) snapshot at construction, so a freshly constructed + /// shell always has a valid, if empty, workspace rather than a null placeholder. + /// + public WorkspaceSnapshot CurrentWorkspace { get; private set; } + + /// + /// Maps each currently loaded source's id to the files it contributed, mirroring the most recent + /// . Exposed read-only so + /// can build its source/file tree without needing its own + /// instance (the shell is the single owner of source state). /// - public WorkspaceSnapshot? CurrentWorkspace { get; private set; } + public IReadOnlyDictionary> CurrentSourceIdToFiles { get; private set; } = + new Dictionary>(StringComparer.Ordinal); /// /// Selected catalog view, if the user is in predefined-view mode. @@ -129,11 +153,24 @@ public sealed class MainWindowShell : IDisposable /// /// Raised via the constructor-injected (see ), /// so subscribers are guaranteed to observe it on the dispatcher's target thread even though the shell - /// methods that trigger it - most notably , which awaits workspace + /// methods that trigger it - most notably , which awaits workspace /// loading with ConfigureAwait(false) - may themselves resume on a background thread pool thread. /// public event EventHandler? TabsChanged; + /// + /// Raised whenever the set of workspace sources changes: a file or folder source is added or removed. + /// Raised after the resulting workspace snapshot has already been applied ( + /// and reflect the change), so subscribers such as + /// can rebuild their source tree directly from shell state. + /// + /// + /// Raised via the same injected as , for the same + /// reason: source mutators await workspace loading with ConfigureAwait(false) and may resume on a + /// background thread. + /// + public event EventHandler? SourcesChanged; + /// /// Dispatcher used to marshal notifications onto whatever thread the shell's /// UI-facing consumers require. @@ -188,6 +225,13 @@ public MainWindowShell( _snippetGenerator = snippetGenerator; _logger = logger; _uiDispatcher = uiDispatcher ?? new ImmediateUiDispatcher(); + + // Eagerly establish a valid, empty (0-source) workspace snapshot at construction, so CurrentWorkspace is + // never null. Safe to await synchronously here: a zero-source resolution's WorkspaceLoader.LoadAsync([]) + // call performs no I/O and does not throw (confirmed stdlib-only, diagnostic-free result), and + // LoadInternalAsync uses ConfigureAwait(false) throughout so this cannot deadlock on a captured context. + var emptySnapshot = _workspaceModel.LoadWorkspaceAsync(_sourceSet.Sources, _sourceSet.Resolve()).GetAwaiter().GetResult(); + CurrentWorkspace = emptySnapshot; } /// @@ -201,62 +245,151 @@ private void RaiseTabsChanged() } /// - /// Loads a new workspace into the shell, refreshes the view catalog and diagnostics, resets active - /// selections, and (re)targets live-watching to the newly opened folder. + /// Raises via the injected , so the + /// notification always reaches subscribers on the dispatcher's target thread, regardless of which thread + /// this method is called from. /// - /// User-selected folder. - /// The freshly loaded workspace snapshot. - /// - /// The file watcher is retargeted to on every call, not just the first: any - /// previously watched root is torn down and any of its pending change state is discarded, so a second or - /// later workspace open never leaves the watcher bound to a stale folder. Because workspace loading is - /// awaited with ConfigureAwait(false), this method's continuation - including the - /// notification raised by - may resume on - /// a background thread; is nonetheless delivered via the injected - /// , so UI-facing subscribers still observe it on their required thread. - /// - /// Thrown when is null or whitespace. - /// Thrown when does not exist. - public async Task OpenWorkspaceAsync(string rootPath) + private void RaiseSourcesChanged() { - ArgumentException.ThrowIfNullOrWhiteSpace(rootPath); + _uiDispatcher.Post(() => SourcesChanged?.Invoke(this, EventArgs.Empty)); + } + + /// + /// Adds a single file to the workspace's source set, or returns the current snapshot unchanged if the + /// same normalized path is already a registered file source. + /// + /// File path to add. + /// The freshly resolved and loaded workspace snapshot. + /// Thrown when is null or whitespace. + public async Task AddFileSourceAsync(string path) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); try { - var snapshot = await _workspaceModel.LoadWorkspaceAsync(rootPath).ConfigureAwait(false); - ApplyWorkspaceSnapshot(snapshot); + _sourceSet.AddFile(path); + var snapshot = await ApplySourceSetChangeAsync().ConfigureAwait(false); + _logger.Log(LogLevel.Info, $"Workspace file source added: {path}"); + return snapshot; + } + catch (Exception ex) when (ex is not ArgumentException) + { + _logger.Log(LogLevel.Error, $"Failed to add workspace file source '{path}'", ex); + throw; + } + } - _fileWatcher.StartWatching(snapshot.RootPath); + /// + /// Adds a folder to the workspace's source set, or returns the current snapshot unchanged if the same + /// normalized path is already a registered folder source. + /// + /// Folder path to add. + /// The freshly resolved and loaded workspace snapshot. + /// Thrown when is null or whitespace. + /// Thrown when does not exist. + public async Task AddFolderSourceAsync(string path) + { + ArgumentException.ThrowIfNullOrWhiteSpace(path); - _logger.Log(LogLevel.Info, $"Workspace opened: {snapshot.RootPath}"); + try + { + _sourceSet.AddFolder(path); + var snapshot = await ApplySourceSetChangeAsync().ConfigureAwait(false); + _logger.Log(LogLevel.Info, $"Workspace folder source added: {path}"); return snapshot; } catch (Exception ex) when (ex is not ArgumentException) { - _logger.Log(LogLevel.Error, $"Failed to open workspace '{rootPath}'", ex); + _logger.Log(LogLevel.Error, $"Failed to add workspace folder source '{path}'", ex); throw; } } + /// + /// Removes a source from the workspace's source set. A no-op (still resolves and reapplies) when + /// does not refer to a currently registered source. + /// + /// Identifier of the source to remove. + /// The freshly resolved and loaded workspace snapshot. + /// Thrown when is null or whitespace. + public async Task RemoveSourceAsync(string sourceId) + { + ArgumentException.ThrowIfNullOrWhiteSpace(sourceId); + + try + { + _sourceSet.RemoveSource(sourceId); + var snapshot = await ApplySourceSetChangeAsync().ConfigureAwait(false); + _logger.Log(LogLevel.Info, $"Workspace source removed: {sourceId}"); + return snapshot; + } + catch (Exception ex) when (ex is not ArgumentException) + { + _logger.Log(LogLevel.Error, $"Failed to remove workspace source '{sourceId}'", ex); + throw; + } + } + + /// + /// Resolves the current , loads the resulting workspace, applies the snapshot, + /// diffs the previous and new watch sets to call / + /// only for sources that actually changed, and raises + /// . + /// + /// The freshly resolved and loaded workspace snapshot. + private async Task ApplySourceSetChangeAsync() + { + var resolution = _sourceSet.Resolve(); + var snapshot = await _workspaceModel.LoadWorkspaceAsync(_sourceSet.Sources, resolution).ConfigureAwait(false); + ApplyWorkspaceSnapshot(snapshot); + CurrentSourceIdToFiles = resolution.SourceIdToFiles; + + var currentSourceIds = _sourceSet.Sources.Select(s => s.Id).ToHashSet(); + + // Start watching every newly added source (idempotent for sources already watched). + foreach (var source in _sourceSet.Sources) + { + if (_watchedSourceIds.Add(source.Id)) + { + _fileWatcher.WatchSource(source); + } + } + + // Stop watching every source no longer present in the set. + foreach (var staleSourceId in _watchedSourceIds.Where(id => !currentSourceIds.Contains(id)).ToList()) + { + _fileWatcher.UnwatchSource(staleSourceId); + _watchedSourceIds.Remove(staleSourceId); + } + + RaiseSourcesChanged(); + return snapshot; + } + /// /// Re-applies pending external file changes by reloading the workspace and refreshing all /// workspace-derived shell state. /// + /// + /// Safe to call with zero currently watched sources: the shell then skips + /// entirely (which itself requires at least one watched + /// source) and reloads against zero changed paths, which is a valid, cheap no-op-ish reload rather than a + /// failure - refreshing an empty workspace is a first-class, supported operation. Always re-resolves + /// before reloading, so a file created or deleted externally under a + /// still-registered folder source is actually picked up, rather than the reload recomputing against a + /// stale file list captured at the last explicit Add/Remove source mutation. + /// /// The refreshed workspace snapshot. - /// Thrown when no workspace has been opened yet. public async Task RefreshFromExternalChangesAsync() { - if (CurrentWorkspace is null) - { - throw new InvalidOperationException("A workspace must be opened before it can be refreshed."); - } - - var changedPaths = _fileWatcher.FlushPendingChanges(); + IReadOnlyList changedPaths = _watchedSourceIds.Count == 0 ? [] : _fileWatcher.FlushPendingChanges(); try { - var snapshot = await _workspaceModel.ReloadFilesAsync(changedPaths).ConfigureAwait(false); + var resolution = _sourceSet.Resolve(); + var snapshot = await _workspaceModel.ReloadFilesAsync(changedPaths, resolution).ConfigureAwait(false); ApplyWorkspaceSnapshot(snapshot); + CurrentSourceIdToFiles = resolution.SourceIdToFiles; _logger.Log(LogLevel.Info, $"Workspace refreshed after external change ({changedPaths.Count} file(s))."); return snapshot; } @@ -278,9 +411,9 @@ public async Task RefreshFromExternalChangesAsync() /// Thrown when is not present in the catalog. public string SelectPredefinedView(string viewId) { - if (CurrentWorkspace is null) + if (CurrentWorkspace.Sources.Count == 0) { - throw new InvalidOperationException("A workspace must be opened before a view can be selected."); + throw new InvalidOperationException("A workspace source must be added before a view can be selected."); } var descriptor = _viewCatalogPresenter.SelectView(viewId); @@ -319,9 +452,9 @@ public string SelectPredefinedView(string viewId) public string PreviewCustomView(ViewDefinitionModel definition) { ArgumentNullException.ThrowIfNull(definition); - if (CurrentWorkspace is null) + if (CurrentWorkspace.Sources.Count == 0) { - throw new InvalidOperationException("A workspace must be opened before a custom view can be previewed."); + throw new InvalidOperationException("A workspace source must be added before a custom view can be previewed."); } var validation = definition.ValidateAgainstWorkspace(CurrentWorkspace.Workspace); diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml index b79b0c9..cd7ff7b 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml @@ -7,13 +7,15 @@ Icon="avares://DemaConsulting.SysML2Workbench/Assets/Icon.png" Width="1280" Height="800"> - + - + + + diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs index 4f459fd..9bef47c 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs @@ -1,4 +1,5 @@ using Avalonia.Controls; +using Avalonia.Input; using Avalonia.Interactivity; using Avalonia.Platform.Storage; using Dock.Model.Core; @@ -22,6 +23,7 @@ public partial class MainWindowView : Window private readonly PredefinedViewsToolViewModel _predefinedViewsViewModel; private readonly CustomViewBuilderToolViewModel _customViewBuilderViewModel; private readonly DiagnosticsToolViewModel _diagnosticsViewModel; + private readonly WorkspacePanelToolViewModel _workspacePanelViewModel; private readonly WorkbenchDockFactory _dockFactory; private readonly Dictionary _diagramViewModelsByTabId = new(); @@ -47,8 +49,9 @@ public MainWindowView(MainWindowShell shell) _predefinedViewsViewModel = new PredefinedViewsToolViewModel(_shell) { Id = "PredefinedViews", Title = "Predefined Views" }; _customViewBuilderViewModel = new CustomViewBuilderToolViewModel(_shell) { Id = "CustomViewBuilder", Title = "Custom View Builder" }; _diagnosticsViewModel = new DiagnosticsToolViewModel(_shell) { Id = "Diagnostics", Title = "Diagnostics" }; + _workspacePanelViewModel = new WorkspacePanelToolViewModel(_shell) { Id = "WorkspacePanel", Title = "Workspace" }; - var factory = new WorkbenchDockFactory(_predefinedViewsViewModel, _customViewBuilderViewModel, _diagnosticsViewModel); + var factory = new WorkbenchDockFactory(_predefinedViewsViewModel, _customViewBuilderViewModel, _diagnosticsViewModel, _workspacePanelViewModel); var layout = factory.CreateLayout(); factory.InitLayout(layout); WorkbenchDockControl.Layout = (IDock)layout; @@ -59,7 +62,8 @@ public MainWindowView(MainWindowShell shell) _shell.TabsChanged += OnShellTabsChanged; OnShellTabsChanged(this, EventArgs.Empty); - OpenWorkspaceMenuItem.Click += OnOpenWorkspaceClick; + AddFileSourceMenuItem.Click += OnAddFileSourceClick; + AddFolderSourceMenuItem.Click += OnAddFolderSourceClick; // Each View-menu item's DataContext is explicitly set to its panel view model (distinct from this // window's own DataContext) so the one-way IsChecked binding above resolves against the panel's @@ -67,6 +71,10 @@ public MainWindowView(MainWindowShell shell) PredefinedViewsMenuItem.DataContext = _predefinedViewsViewModel; CustomViewBuilderMenuItem.DataContext = _customViewBuilderViewModel; DiagnosticsMenuItem.DataContext = _diagnosticsViewModel; + WorkspacePanelMenuItem.DataContext = _workspacePanelViewModel; + + AddHandler(DragDrop.DropEvent, OnWindowDrop); + AddHandler(DragDrop.DragOverEvent, OnWindowDragOver); } /// @@ -85,6 +93,14 @@ private void OnCustomViewBuilderMenuItemClick(object? sender, RoutedEventArgs e) ShowOrFocusPanel(_customViewBuilderViewModel); } + /// + /// Handles the View menu's "Workspace" click by showing or focusing that panel. + /// + private void OnWorkspacePanelMenuItemClick(object? sender, RoutedEventArgs e) + { + ShowOrFocusPanel(_workspacePanelViewModel); + } + /// /// Handles the View menu's "Diagnostics" click by showing or focusing that panel. /// @@ -178,7 +194,38 @@ private void OnShellTabsChanged(object? sender, EventArgs e) } } - private async void OnOpenWorkspaceClick(object? sender, RoutedEventArgs e) + private async void OnAddFileSourceClick(object? sender, RoutedEventArgs e) + { + var topLevel = GetTopLevel(this); + if (topLevel?.StorageProvider is null) + { + return; + } + + var files = await topLevel.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions + { + Title = "Open SysML2 File", + AllowMultiple = false, + }); + + var filePath = files.Count > 0 ? files[0].TryGetLocalPath() : null; + if (string.IsNullOrEmpty(filePath)) + { + return; + } + + try + { + await _shell.AddFileSourceAsync(filePath); + RefreshPanelsFromWorkspace(); + } + catch (Exception ex) + { + _customViewBuilderViewModel.StatusMessage = $"Failed to open file: {ex.Message}"; + } + } + + private async void OnAddFolderSourceClick(object? sender, RoutedEventArgs e) { var topLevel = GetTopLevel(this); if (topLevel?.StorageProvider is null) @@ -188,7 +235,7 @@ private async void OnOpenWorkspaceClick(object? sender, RoutedEventArgs e) var folders = await topLevel.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions { - Title = "Open SysML2 Workspace", + Title = "Open SysML2 Folder", AllowMultiple = false, }); @@ -200,14 +247,69 @@ private async void OnOpenWorkspaceClick(object? sender, RoutedEventArgs e) try { - await _shell.OpenWorkspaceAsync(folderPath); - _predefinedViewsViewModel.RefreshFromWorkspace(); - _customViewBuilderViewModel.RefreshFromWorkspace(); - _diagnosticsViewModel.RefreshFromWorkspace(); + await _shell.AddFolderSourceAsync(folderPath); + RefreshPanelsFromWorkspace(); } catch (Exception ex) { - _customViewBuilderViewModel.StatusMessage = $"Failed to open workspace: {ex.Message}"; + _customViewBuilderViewModel.StatusMessage = $"Failed to open folder: {ex.Message}"; } } + + /// + /// Handles a drag-and-drop drop anywhere on the main window by adding each dropped file or folder as a + /// new workspace source, funneling through the same APIs the File menu and + /// Workspace panel use - no separate drag-and-drop orchestration. + /// + private async void OnWindowDrop(object? sender, DragEventArgs e) + { + if (!e.DataTransfer.Formats.Contains(DataFormat.File)) + { + return; + } + + foreach (var item in e.DataTransfer.TryGetFiles() ?? []) + { + var path = item.TryGetLocalPath(); + if (string.IsNullOrEmpty(path)) + { + continue; + } + + try + { + if (Directory.Exists(path)) + { + await _shell.AddFolderSourceAsync(path); + } + else if (File.Exists(path)) + { + await _shell.AddFileSourceAsync(path); + } + } + catch (Exception ex) + { + _customViewBuilderViewModel.StatusMessage = $"Failed to open dropped path '{path}': {ex.Message}"; + } + } + + RefreshPanelsFromWorkspace(); + } + + private void OnWindowDragOver(object? sender, DragEventArgs e) + { + e.DragEffects = e.DataTransfer.Formats.Contains(DataFormat.File) ? DragDropEffects.Copy : DragDropEffects.None; + } + + /// + /// Refreshes the three workspace-derived tool panels after a source is added or removed through the File + /// menu or a drag-and-drop drop. The Workspace panel itself refreshes independently via its own + /// subscription. + /// + private void RefreshPanelsFromWorkspace() + { + _predefinedViewsViewModel.RefreshFromWorkspace(); + _customViewBuilderViewModel.RefreshFromWorkspace(); + _diagnosticsViewModel.RefreshFromWorkspace(); + } } diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/PredefinedViewsToolView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/PredefinedViewsToolView.axaml index c7e8e03..a2906e6 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/PredefinedViewsToolView.axaml +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/PredefinedViewsToolView.axaml @@ -7,6 +7,7 @@ + /// Creates the predefined-views tool view model. /// @@ -31,6 +34,8 @@ public partial class PredefinedViewsToolViewModel : Dock.Model.Mvvm.Controls.Too public PredefinedViewsToolViewModel(MainWindowShell shell) { _shell = shell ?? throw new ArgumentNullException(nameof(shell)); + _shell.SourcesChanged += (_, _) => RefreshFromWorkspace(); + RefreshFromWorkspace(); } /// @@ -41,6 +46,7 @@ public void RefreshFromWorkspace() AvailableViews = _shell.ViewCatalog.AvailableViews; SelectedView = null; StatusMessage = null; + IsWorkspaceEmpty = _shell.CurrentWorkspace.Sources.Count == 0; } partial void OnSelectedViewChanged(ViewDescriptor? value) diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkbenchDockFactory.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkbenchDockFactory.cs index 3e5d842..760ad7a 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkbenchDockFactory.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkbenchDockFactory.cs @@ -6,10 +6,13 @@ namespace DemaConsulting.SysML2Workbench.AppShellSubsystem; /// -/// Composes the four Phase-0 panels into a resizable/floatable/closable Dock layout, approximating the -/// legacy fixed-DockPanel arrangement's default proportions (left ~260px, right ~320px, bottom ~180px -/// against the window's default 1280x800 size) as initial -/// proportions, while leaving every panel user-resizable, floatable, and closable through Dock's own chrome. +/// Composes the four Phase-0 panels plus the Workspace sources panel into a resizable/floatable/closable +/// Dock layout, approximating the legacy fixed-DockPanel arrangement's default proportions (left +/// ~260px, right ~320px, bottom ~180px against the window's default 1280x800 size) as initial +/// proportions, while leaving every panel user-resizable, floatable, +/// and closable through Dock's own chrome. The Workspace panel shares the Left column with the Predefined +/// Views panel as a second tab (rather than its own column), keeping the other three panes' proportions +/// unchanged. /// HideToolsOnClose is set so that closing a hides it /// (tracked in and restorable via /// RestoreDockable) rather than permanently removing it, so the "View" menu can bring a closed panel @@ -31,6 +34,7 @@ public sealed class WorkbenchDockFactory : Factory private readonly PredefinedViewsToolViewModel _predefinedViewsViewModel; private readonly CustomViewBuilderToolViewModel _customViewBuilderViewModel; private readonly DiagnosticsToolViewModel _diagnosticsViewModel; + private readonly WorkspacePanelToolViewModel _workspacePanelViewModel; /// /// Raised after a is closed through Dock's own chrome (or any @@ -40,21 +44,24 @@ public sealed class WorkbenchDockFactory : Factory public event EventHandler? DiagramTabClosed; /// - /// Creates the dock layout factory over the three already-constructed Tool panel view models. The diagram + /// Creates the dock layout factory over the four already-constructed Tool panel view models. The diagram /// is populated dynamically at runtime rather than at construction time - see /// . /// /// Predefined-views tool panel. /// Custom-view builder tool panel. /// Diagnostics tool panel. + /// Workspace sources tool panel. public WorkbenchDockFactory( PredefinedViewsToolViewModel predefinedViewsViewModel, CustomViewBuilderToolViewModel customViewBuilderViewModel, - DiagnosticsToolViewModel diagnosticsViewModel) + DiagnosticsToolViewModel diagnosticsViewModel, + WorkspacePanelToolViewModel workspacePanelViewModel) { _predefinedViewsViewModel = predefinedViewsViewModel ?? throw new ArgumentNullException(nameof(predefinedViewsViewModel)); _customViewBuilderViewModel = customViewBuilderViewModel ?? throw new ArgumentNullException(nameof(customViewBuilderViewModel)); _diagnosticsViewModel = diagnosticsViewModel ?? throw new ArgumentNullException(nameof(diagnosticsViewModel)); + _workspacePanelViewModel = workspacePanelViewModel ?? throw new ArgumentNullException(nameof(workspacePanelViewModel)); HideToolsOnClose = true; } @@ -74,7 +81,7 @@ public override IRootDock CreateLayout() Id = "PredefinedViewsPane", Alignment = Alignment.Left, Proportion = 0.20, - VisibleDockables = CreateList(_predefinedViewsViewModel), + VisibleDockables = CreateList(_predefinedViewsViewModel, _workspacePanelViewModel), ActiveDockable = _predefinedViewsViewModel, }; diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml new file mode 100644 index 0000000..b99cb45 --- /dev/null +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml @@ -0,0 +1,40 @@ + + + + + + + public required string Id { get; init; } + + /// + /// The icon shown alongside this node in the tree. + /// + public abstract MaterialIconKind IconKind { get; } } /// @@ -32,6 +38,10 @@ public sealed class WorkspaceSourceNode : WorkspaceTreeNode /// Files contributed by this source. Empty for a source. /// public required IReadOnlyList Children { get; init; } + + /// + public override MaterialIconKind IconKind => + Source.Kind == WorkspaceSourceKind.Folder ? MaterialIconKind.Folder : MaterialIconKind.FileOutline; } /// @@ -50,6 +60,9 @@ public sealed class WorkspaceFileNode : WorkspaceTreeNode /// Identifier of the that owns this file. /// public required string SourceId { get; init; } + + /// + public override MaterialIconKind IconKind => MaterialIconKind.FileOutline; } /// diff --git a/src/DemaConsulting.SysML2Workbench/DemaConsulting.SysML2Workbench.csproj b/src/DemaConsulting.SysML2Workbench/DemaConsulting.SysML2Workbench.csproj index f52aa8d..f612895 100644 --- a/src/DemaConsulting.SysML2Workbench/DemaConsulting.SysML2Workbench.csproj +++ b/src/DemaConsulting.SysML2Workbench/DemaConsulting.SysML2Workbench.csproj @@ -10,6 +10,7 @@ + From 70ef2462145dbedc22444bf3da454275c21f7ab6 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 19:39:01 -0400 Subject: [PATCH 07/14] Add menu icons and a File > Exit item Wired MaterialIcon icons (Material.Icons.Avalonia, already added for the Workspace tree) into every File and View menu item, and added a File > Exit item that closes the main window (the desktop app's only window, so this cleanly shuts down the application). Verified: dotnet build (0 warnings/errors), dotnet test (151/151), pwsh ./fix.ps1 (clean, only the 2 intended files changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AppShellSubsystem/MainWindowView.axaml | 43 ++++++++++++++++--- .../AppShellSubsystem/MainWindowView.axaml.cs | 9 ++++ 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml index cd7ff7b..156c36c 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml @@ -2,6 +2,7 @@ xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:dock="using:Dock.Avalonia.Controls" xmlns:dockControls="using:Dock.Model.Mvvm.Controls" + xmlns:mi="using:Material.Icons.Avalonia" x:Class="DemaConsulting.SysML2Workbench.AppShellSubsystem.MainWindowView" Title="SysML2Workbench" Icon="avares://DemaConsulting.SysML2Workbench/Assets/Icon.png" @@ -11,14 +12,44 @@ - - + + + + + + + + + + + + + + + + - - - - + + + + + + + + + + + + + + + + + + + + diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs index 16ac2d5..613b1d2 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs @@ -114,6 +114,15 @@ private void OnDiagnosticsMenuItemClick(object? sender, RoutedEventArgs e) ShowOrFocusPanel(_diagnosticsViewModel); } + /// + /// Handles the File menu's "Exit" click by closing the main window, which shuts down the application + /// (this is the desktop app's only window). + /// + private void OnExitMenuItemClick(object? sender, RoutedEventArgs e) + { + Close(); + } + /// /// Restores to its original dock if it was hidden by a prior close (via /// 's HideToolsOnClose setting, a safe no-op if it is not From 8846d39a6f335d379929e6503e09d63bff0830e2 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 19:43:35 -0400 Subject: [PATCH 08/14] Register MaterialIconStyles so MaterialIcon controls actually render Material.Icons.Avalonia requires its MaterialIconStyles to be added to Application.Styles (App.axaml) - without it, MaterialIcon renders nothing (no glyph, no reserved space), which is why the Workspace tree, File menu, and View menu icons added in prior commits weren't visible despite building and running fine. Added alongside the existing FluentTheme and DockFluentTheme. Verified: dotnet build (0 warnings/errors), dotnet test (151/151), pwsh ./fix.ps1 (clean, only App.axaml changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- src/DemaConsulting.SysML2Workbench/App.axaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/DemaConsulting.SysML2Workbench/App.axaml b/src/DemaConsulting.SysML2Workbench/App.axaml index 97178d6..d750ae0 100644 --- a/src/DemaConsulting.SysML2Workbench/App.axaml +++ b/src/DemaConsulting.SysML2Workbench/App.axaml @@ -1,6 +1,7 @@ @@ -11,6 +12,7 @@ + From 543873bc492cf88346cf1f7b74a261ad4be6783e Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 19:56:41 -0400 Subject: [PATCH 09/14] Preserve on-disk folder hierarchy in the Workspace tree A folder source's discovered files were flattened directly under the source node - expanding a folder showed every file it contained as a single flat list, defeating the point of the tree being expandable at all (navigating to a specific file in a large/deeply nested folder was effectively impossible). - Added WorkspaceFolderNode: an intermediate, purely-display tree node for one subfolder, carrying its own Name and nested Children (no removal/watch identity of its own - only whole sources are removable, and the owning source's watch scope already covers everything beneath it). - WorkspaceSourceNode.Children is now IReadOnlyList (was IReadOnlyList): a folder source's top-level children are now a mix of WorkspaceFolderNode/WorkspaceFileNode instead of a flat file list. - Added WorkspacePanelToolViewModel.BuildFolderChildren, grouping a source's flat discovered-file list (absolute paths) by each file's position relative to the source's own path, recursively, into that nested shape - subfolders sorted before files, each level sorted alphabetically by name. - WorkspaceFileNode gained a computed Name (Path.GetFileName(FilePath)) shown in the tree instead of the full absolute path, since ancestor nodes now already convey the directory; WorkspaceFolderNode.Name is likewise just the folder's own last path segment. - Updated WorkspacePanelToolView's TreeDataTemplates: added a template for WorkspaceFolderNode, and switched WorkspaceFileNode's text binding from FilePath to Name. - Added a regression test (RebuildTree_FolderSourceWithSubfolders_PreservesOnDiskHierarchy) covering a 2-level-deep nested folder structure, and updated the companion design/verification/reqstream docs. Verified: dotnet build (0 warnings/errors), dotnet test (152/152, up from 151), dotnet sysml2tools lint (clean), dotnet reqstream --lint (clean), pwsh ./fix.ps1 (clean, only the 6 intended files changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../app-shell-subsystem/workspace-panel.md | 43 ++++-- .../app-shell-subsystem/workspace-panel.yaml | 5 +- .../app-shell-subsystem/workspace-panel.md | 6 + .../WorkspacePanelToolView.axaml | 8 +- .../WorkspacePanelToolViewModel.cs | 130 ++++++++++++++++-- .../WorkspacePanelToolViewModelTests.cs | 42 +++++- 6 files changed, 210 insertions(+), 24 deletions(-) diff --git a/docs/design/sysml2-workbench/app-shell-subsystem/workspace-panel.md b/docs/design/sysml2-workbench/app-shell-subsystem/workspace-panel.md index aec854a..8ac3bb8 100644 --- a/docs/design/sysml2-workbench/app-shell-subsystem/workspace-panel.md +++ b/docs/design/sysml2-workbench/app-shell-subsystem/workspace-panel.md @@ -19,18 +19,31 @@ view), matching the pairing convention used by the other three tool panels per workspace source, in the same order as `MainWindowShell.CurrentWorkspace.Sources`. **WorkspaceTreeNode**: abstract base with `required string Id { get; init; }` -— the tree control's item key. +— the tree control's item key — and `abstract MaterialIconKind IconKind`, the +icon shown alongside the node. **WorkspaceSourceNode**: `WorkspaceTreeNode` with `required WorkspaceSource Source` -and `required IReadOnlyList Children` — a source's node. -`Children` is empty for a `File`-kind source (a leaf, no expand arrow) and one -entry per discovered file for a `Folder`-kind source. - -**WorkspaceFileNode**: `WorkspaceTreeNode` with `required string FilePath` and -`required string SourceId` — a leaf node for one file. `FilePath` is a stable -identity intended for a future, out-of-scope, double-click read-only file -viewer; it is preserved even though nothing else currently reads it beyond -display. +and `required IReadOnlyList Children` — a source's node. +`Children` is empty for a `File`-kind source (a leaf, no expand arrow); for a +`Folder`-kind source it holds one entry per top-level subfolder +(`WorkspaceFolderNode`) or file (`WorkspaceFileNode`) directly under that +folder, preserving its on-disk hierarchy rather than flattening every +discovered file into a single list. + +**WorkspaceFolderNode**: `WorkspaceTreeNode` with `required string Name` (the +folder's own last path segment, not its full path) and +`required IReadOnlyList Children` — an intermediate, +purely-display grouping node for one subfolder discovered under a +`Folder`-kind source. It carries no identity used for removal (only whole +sources are removable) or file-watching (the owning source's watch scope +already covers everything beneath it). + +**WorkspaceFileNode**: `WorkspaceTreeNode` with `required string FilePath`, +`required string SourceId`, and a computed `Name` (`Path.GetFileName(FilePath)`, +shown in the tree since ancestor nodes already convey the file's directory) — +a leaf node for one file. `FilePath` is a stable identity intended for a +future, out-of-scope, double-click read-only file viewer; it is preserved +even though nothing else currently reads it beyond display. **SelectedNode**: `WorkspaceTreeNode?` — the tree node currently selected by the user, used to resolve which source `RemoveSelected` acts on. @@ -53,8 +66,14 @@ state. with the correct `Children` shape for its kind; overlap dedupe already resolved upstream by `WorkspaceSourceSet.Resolve()` is reflected as a single attribution in the tree shape (a deduplicated file appears once, under its - attributed owning source, not once per overlapping source). Called eagerly - from the constructor and every time `MainWindowShell.SourcesChanged` fires. + attributed owning source, not once per overlapping source). A folder + source's flat, per-source file list is grouped by each file's position + relative to that source's own path, via a private `BuildFolderChildren` + helper, so the resulting `Children` mirror the folder's on-disk + hierarchy — subfolders (as `WorkspaceFolderNode`s, sorted before files) and + files (as `WorkspaceFileNode`s), each level sorted alphabetically by name. + Called eagerly from the constructor and every time + `MainWindowShell.SourcesChanged` fires. **AddFile** / **AddFolder**: Commands that raise `RequestAddFile` / `RequestAddFolder` so the Avalonia-aware view can fulfill them with a real diff --git a/docs/reqstream/sysml2-workbench/app-shell-subsystem/workspace-panel.yaml b/docs/reqstream/sysml2-workbench/app-shell-subsystem/workspace-panel.yaml index a197b2e..db4ac85 100644 --- a/docs/reqstream/sysml2-workbench/app-shell-subsystem/workspace-panel.yaml +++ b/docs/reqstream/sysml2-workbench/app-shell-subsystem/workspace-panel.yaml @@ -7,13 +7,14 @@ sections: - title: 'WorkspacePanel Requirements' requirements: - id: 'SysML2Workbench-AppShellSubsystem-WorkspacePanel-BuildSourceTree' - title: 'The WorkspacePanel shall present the current workspace sources as a tree, with folder sources expanding to their discovered files and file sources as leaves.' + title: 'The WorkspacePanel shall present the current workspace sources as a tree, with folder sources expanding to their discovered files preserving on-disk subfolder hierarchy, and file sources as leaves.' justification: | - Users need a visual inventory of every open source and, for folders, the files contributed by each one, distinguishing expandable folder nodes from non-expandable file nodes. + Users need a visual inventory of every open source and, for folders, the files contributed by each one arranged the way they are actually organized on disk (not flattened), distinguishing expandable folder nodes from non-expandable file nodes. tests: - 'Construction_ZeroSources_ReportsEmptyTree' - 'RebuildTree_FolderSource_ProducesSourceNodeWithFileChildren' - 'RebuildTree_FileSource_ProducesLeafSourceNodeWithNoChildren' + - 'RebuildTree_FolderSourceWithSubfolders_PreservesOnDiskHierarchy' - id: 'SysML2Workbench-AppShellSubsystem-WorkspacePanel-ReflectDedupeInTreeShape' title: 'The WorkspacePanel shall reflect WorkspaceSourceSet overlap dedupe in its tree shape, showing an overlapping file exactly once under its attributed owning source.' diff --git a/docs/verification/sysml2-workbench/app-shell-subsystem/workspace-panel.md b/docs/verification/sysml2-workbench/app-shell-subsystem/workspace-panel.md index 8c2acf1..73943ac 100644 --- a/docs/verification/sysml2-workbench/app-shell-subsystem/workspace-panel.md +++ b/docs/verification/sysml2-workbench/app-shell-subsystem/workspace-panel.md @@ -39,6 +39,12 @@ produces an empty `RootNodes` list and `IsEmpty == true`. Verified by `WorkspaceSourceNode` whose `Children` list contains one `WorkspaceFileNode` per file discovered under that folder. Verified by `WorkspacePanelToolViewModelTests.RebuildTree_FolderSource_ProducesSourceNodeWithFileChildren`. +**RebuildTree_FolderSourceWithSubfolders_PreservesOnDiskHierarchy**: Adding a folder source whose discovered files +span multiple nested subfolders produces intermediate `WorkspaceFolderNode`s mirroring that on-disk hierarchy +(subfolders before files, each level sorted alphabetically) instead of flattening every file directly under the +source node. Verified by +`WorkspacePanelToolViewModelTests.RebuildTree_FolderSourceWithSubfolders_PreservesOnDiskHierarchy`. + **RebuildTree_FileSource_ProducesLeafSourceNodeWithNoChildren**: Adding a file source and rebuilding produces a `WorkspaceSourceNode` with an empty `Children` list (a leaf, no expand arrow). Verified by `WorkspacePanelToolViewModelTests.RebuildTree_FileSource_ProducesLeafSourceNodeWithNoChildren`. diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml index 421e5ec..603f4f3 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml @@ -34,10 +34,16 @@ + + + + + + - + diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs index 30395a2..b9f33d4 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs @@ -23,9 +23,10 @@ public abstract class WorkspaceTreeNode /// /// A tree node representing one . Has no children for a -/// source (a leaf, with no expand arrow); has one -/// child per discovered file for a -/// source. +/// source (a leaf, with no expand arrow); has one child per +/// top-level entry (subfolder or file) discovered under a source, +/// preserving that folder's on-disk hierarchy rather than flattening every discovered file into a single +/// list (see ). /// public sealed class WorkspaceSourceNode : WorkspaceTreeNode { @@ -35,15 +36,39 @@ public sealed class WorkspaceSourceNode : WorkspaceTreeNode public required WorkspaceSource Source { get; init; } /// - /// Files contributed by this source. Empty for a source. + /// Top-level children (subfolders and/or files) contributed by this source. Empty for a + /// source. /// - public required IReadOnlyList Children { get; init; } + public required IReadOnlyList Children { get; init; } /// public override MaterialIconKind IconKind => Source.Kind == WorkspaceSourceKind.Folder ? MaterialIconKind.Folder : MaterialIconKind.FileOutline; } +/// +/// An intermediate tree node representing one subfolder discovered under a +/// source, preserving that subfolder's on-disk position instead of flattening its files up to the source +/// node. Purely a display grouping - it carries no identity used for removal (only whole sources are +/// removable) or file-watching (the source's watch scope already covers everything beneath it). +/// +public sealed class WorkspaceFolderNode : WorkspaceTreeNode +{ + /// + /// The folder's own name (the last path segment), not its full path. + /// + public required string Name { get; init; } + + /// + /// This folder's children: nested s for its subfolders, and + /// s for files directly within it. + /// + public required IReadOnlyList Children { get; init; } + + /// + public override MaterialIconKind IconKind => MaterialIconKind.Folder; +} + /// /// A leaf tree node representing one file contributed by a source. Its is a stable /// identity intended for a future (out of scope for this feature) double-click read-only file viewer; it must @@ -61,6 +86,13 @@ public sealed class WorkspaceFileNode : WorkspaceTreeNode /// public required string SourceId { get; init; } + /// + /// The file's own name (the last path segment), shown in the tree instead of its full path since + /// ancestor / nodes already convey + /// its directory. + /// + public string Name => Path.GetFileName(FilePath); + /// public override MaterialIconKind IconKind => MaterialIconKind.FileOutline; } @@ -134,9 +166,7 @@ public void RebuildTree() ? sourceFiles : []; - var children = files - .Select(file => new WorkspaceFileNode { Id = $"{source.Id}::{file}", FilePath = file, SourceId = source.Id }) - .ToList(); + var children = BuildFolderChildren(source, files); return (WorkspaceTreeNode)new WorkspaceSourceNode { Id = source.Id, Source = source, Children = children }; }) @@ -145,6 +175,90 @@ public void RebuildTree() IsEmpty = RootNodes.Count == 0; } + /// + /// Builds the nested subfolder/file children of a source, + /// grouping the flat list (absolute paths) by their position relative to + /// 's own path so the tree preserves that folder's on-disk hierarchy instead + /// of flattening every discovered file directly under the source node. + /// + /// The source the files were discovered under. + /// Absolute paths of every file discovered under . + private static IReadOnlyList BuildFolderChildren(WorkspaceSource source, IReadOnlyList files) + { + if (files.Count == 0) + { + return []; + } + + var root = new FolderGroup(RelativePath: string.Empty); + + foreach (var file in files) + { + var relative = Path.GetRelativePath(source.Path, file); + var segments = relative.Split([Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar], StringSplitOptions.RemoveEmptyEntries); + + var current = root; + for (var i = 0; i < segments.Length - 1; i++) + { + if (!current.Subfolders.TryGetValue(segments[i], out var next)) + { + next = new FolderGroup(RelativePath: current.RelativePath.Length == 0 ? segments[i] : $"{current.RelativePath}/{segments[i]}"); + current.Subfolders[segments[i]] = next; + } + + current = next; + } + + current.Files.Add(file); + } + + return ToTreeNodes(root, source.Id); + } + + /// + /// Converts a built by into the immutable + /// shape the tree binds to, listing subfolders before files and sorting + /// each alphabetically by name. + /// + private static IReadOnlyList ToTreeNodes(FolderGroup group, string sourceId) + { + var nodes = new List(); + + foreach (var (name, subfolder) in group.Subfolders.OrderBy(entry => entry.Key, StringComparer.OrdinalIgnoreCase)) + { + nodes.Add(new WorkspaceFolderNode + { + Id = $"{sourceId}::{subfolder.RelativePath}", + Name = name, + Children = ToTreeNodes(subfolder, sourceId), + }); + } + + foreach (var file in group.Files.OrderBy(Path.GetFileName, StringComparer.OrdinalIgnoreCase)) + { + nodes.Add(new WorkspaceFileNode { Id = $"{sourceId}::{file}", FilePath = file, SourceId = sourceId }); + } + + return nodes; + } + + /// + /// A mutable, in-progress grouping of one folder level while walks a + /// source's flat discovered-file list; converted to immutable s by + /// once fully populated. is this folder's own + /// path relative to the source root (empty for the root group itself), kept so descendant node ids stay + /// unique even when two different subfolders share the same leaf name (for example a/sub and + /// b/sub). + /// + private sealed class FolderGroup(string RelativePath) + { + public string RelativePath { get; } = RelativePath; + + public Dictionary Subfolders { get; } = new(StringComparer.OrdinalIgnoreCase); + + public List Files { get; } = []; + } + /// /// Raises so the view can present a file picker. /// diff --git a/test/DemaConsulting.SysML2Workbench.Tests/AppShellSubsystem/WorkspacePanelToolViewModelTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/AppShellSubsystem/WorkspacePanelToolViewModelTests.cs index 4f470d7..93f07c9 100644 --- a/test/DemaConsulting.SysML2Workbench.Tests/AppShellSubsystem/WorkspacePanelToolViewModelTests.cs +++ b/test/DemaConsulting.SysML2Workbench.Tests/AppShellSubsystem/WorkspacePanelToolViewModelTests.cs @@ -94,10 +94,50 @@ public async Task RebuildTree_FolderSource_ProducesSourceNodeWithFileChildren() var sourceNode = Assert.IsType(Assert.Single(viewModel.RootNodes)); Assert.Equal(WorkspaceSourceKind.Folder, sourceNode.Source.Kind); Assert.Equal(2, sourceNode.Children.Count); - Assert.All(sourceNode.Children, child => Assert.Equal(sourceNode.Source.Id, child.SourceId)); + Assert.All(sourceNode.Children, child => Assert.Equal(sourceNode.Source.Id, Assert.IsType(child).SourceId)); Assert.False(viewModel.IsEmpty); } + /// + /// Validates that files discovered in subfolders are grouped under intermediate + /// s mirroring the on-disk hierarchy, instead of being flattened + /// directly under the source node - this is the whole point of the tree being expandable rather than a + /// flat file list. + /// + [Fact] + public async Task RebuildTree_FolderSourceWithSubfolders_PreservesOnDiskHierarchy() + { + // Arrange: A.sysml directly under the root, B.sysml one level down in "Sub", C.sysml two levels down. + Directory.CreateDirectory(Path.Combine(_tempRoot, "Sub", "Nested")); + await WriteFileAsync(Path.Combine(_tempRoot, "A.sysml"), "package A {\n part def Widget;\n}\n"); + await WriteFileAsync(Path.Combine(_tempRoot, "Sub", "B.sysml"), "package B {\n part def Gadget;\n}\n"); + await WriteFileAsync(Path.Combine(_tempRoot, "Sub", "Nested", "C.sysml"), "package C {\n part def Doohickey;\n}\n"); + using var shell = CreateShell(); + var viewModel = new WorkspacePanelToolViewModel(shell); + + // Act + await shell.AddFolderSourceAsync(_tempRoot); + + // Assert: the source node lists "Sub" (a folder) before A.sysml (a file) - folders sort before files. + var sourceNode = Assert.IsType(Assert.Single(viewModel.RootNodes)); + Assert.Equal(2, sourceNode.Children.Count); + var subFolder = Assert.IsType(sourceNode.Children[0]); + Assert.Equal("Sub", subFolder.Name); + var rootFile = Assert.IsType(sourceNode.Children[1]); + Assert.Equal("A.sysml", rootFile.Name); + + // Assert: "Sub" lists the "Nested" subfolder before B.sysml. + Assert.Equal(2, subFolder.Children.Count); + var nestedFolder = Assert.IsType(subFolder.Children[0]); + Assert.Equal("Nested", nestedFolder.Name); + var subFile = Assert.IsType(subFolder.Children[1]); + Assert.Equal("B.sysml", subFile.Name); + + // Assert: "Nested" contains only C.sysml. + var nestedFile = Assert.IsType(Assert.Single(nestedFolder.Children)); + Assert.Equal("C.sysml", nestedFile.Name); + } + /// /// Validates that a file source's tree node is a leaf: it has zero children, distinguishing it from a /// folder source with zero discovered files. From cf500365025069a44f851a3af6f0aae7a8a80a2f Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 20:10:23 -0400 Subject: [PATCH 10/14] Fix crash-safety issues found by code review Two genuine issues found by a full-branch code-review pass ahead of the PR: 1. MainWindowShell.ApplySourceSetChangeAsync watched newly added sources only after already applying the new workspace snapshot and updating CurrentSourceIdToFiles. If FileWatcher.WatchSource threw (for example DirectoryNotFoundException - a real TOCTOU race if a folder is deleted or becomes inaccessible between WorkspaceSourceSet.AddFolder's own upfront check and this call), the failing source's id had already been marked watched, so it could never be retried, while the source itself was never rolled back out of the source set - it became invisible in the UI (SourcesChanged never fired) yet permanently unwatched and still counted as a live source. Reordered so newly added sources are watched first, before any other state changes; a watch failure now rolls the source back out of the set and rethrows before touching CurrentWorkspace/CurrentSourceIdToFiles/SourcesChanged, so shell state stays fully consistent with the rollback. 2. Four async void picker-driven handlers (MainWindowView's OnAddFileSourceClick/OnAddFolderSourceClick and WorkspacePanelToolView's OnRequestAddFile/OnRequestAddFolder) only wrapped the call into the shell in a try/catch - the preceding StorageProvider.OpenFilePickerAsync/OpenFolderPickerAsync call itself was left outside the try. Since the app has no global unhandled-exception handler, a picker failure (a known real-world occurrence, e.g. missing desktop portal/D-Bus service on some Linux configurations) would crash the whole app instead of surfacing via the existing StatusMessage mechanism. Widened all four try blocks to cover the picker call too. Verified: dotnet build (0 warnings/errors), dotnet test (152/152, no regression from the watch-loop reordering), pwsh ./fix.ps1 (clean, only the 3 intended files changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AppShellSubsystem/MainWindowShell.cs | 41 +++++++++++++---- .../AppShellSubsystem/MainWindowView.axaml.cs | 46 +++++++++---------- .../WorkspacePanelToolView.axaml.cs | 46 +++++++++---------- 3 files changed, 78 insertions(+), 55 deletions(-) diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs index cc4eacf..bb6455b 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowShell.cs @@ -337,24 +337,47 @@ public async Task RemoveSourceAsync(string sourceId) /// . /// /// The freshly resolved and loaded workspace snapshot. + /// + /// Newly added sources are watched before any snapshot/dictionary state is applied and before + /// is raised. can throw (for + /// example if a folder was deleted or became inaccessible in + /// the narrow window between 's own upfront check and this + /// call - a real TOCTOU race, not a theoretical one). If it does, the failing source is rolled back out + /// of and the exception rethrown before any other shell state is touched, so + /// the shell never ends up with a source that is registered in yet invisible in + /// the UI (which only rebuilds from ) and permanently unwatchable (once its + /// id were marked watched despite the watcher never having been created). + /// private async Task ApplySourceSetChangeAsync() { - var resolution = _sourceSet.Resolve(); - var snapshot = await _workspaceModel.LoadWorkspaceAsync(_sourceSet.Sources, resolution).ConfigureAwait(false); - ApplyWorkspaceSnapshot(snapshot); - CurrentSourceIdToFiles = resolution.SourceIdToFiles; - - var currentSourceIds = _sourceSet.Sources.Select(s => s.Id).ToHashSet(); - - // Start watching every newly added source (idempotent for sources already watched). + // Start watching every newly added source (idempotent for sources already watched) before applying any + // other state, so a watch failure can be rolled back cleanly - see remarks above. foreach (var source in _sourceSet.Sources) { - if (_watchedSourceIds.Add(source.Id)) + if (_watchedSourceIds.Contains(source.Id)) + { + continue; + } + + try { _fileWatcher.WatchSource(source); + _watchedSourceIds.Add(source.Id); + } + catch (Exception) + { + _sourceSet.RemoveSource(source.Id); + throw; } } + var resolution = _sourceSet.Resolve(); + var snapshot = await _workspaceModel.LoadWorkspaceAsync(_sourceSet.Sources, resolution).ConfigureAwait(false); + ApplyWorkspaceSnapshot(snapshot); + CurrentSourceIdToFiles = resolution.SourceIdToFiles; + + var currentSourceIds = _sourceSet.Sources.Select(s => s.Id).ToHashSet(); + // Stop watching every source no longer present in the set. foreach (var staleSourceId in _watchedSourceIds.Where(id => !currentSourceIds.Contains(id)).ToList()) { diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs index 613b1d2..56b154d 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/MainWindowView.axaml.cs @@ -216,21 +216,21 @@ private async void OnAddFileSourceClick(object? sender, RoutedEventArgs e) return; } - var files = await topLevel.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions + try { - Title = "Open SysML2 File", - AllowMultiple = false, - FileTypeFilter = [SysmlFileType], - }); + var files = await topLevel.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions + { + Title = "Open SysML2 File", + AllowMultiple = false, + FileTypeFilter = [SysmlFileType], + }); - var filePath = files.Count > 0 ? files[0].TryGetLocalPath() : null; - if (string.IsNullOrEmpty(filePath)) - { - return; - } + var filePath = files.Count > 0 ? files[0].TryGetLocalPath() : null; + if (string.IsNullOrEmpty(filePath)) + { + return; + } - try - { await _shell.AddFileSourceAsync(filePath); RefreshPanelsFromWorkspace(); } @@ -248,20 +248,20 @@ private async void OnAddFolderSourceClick(object? sender, RoutedEventArgs e) return; } - var folders = await topLevel.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions + try { - Title = "Open SysML2 Folder", - AllowMultiple = false, - }); + var folders = await topLevel.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions + { + Title = "Open SysML2 Folder", + AllowMultiple = false, + }); - var folderPath = folders.Count > 0 ? folders[0].TryGetLocalPath() : null; - if (string.IsNullOrEmpty(folderPath)) - { - return; - } + var folderPath = folders.Count > 0 ? folders[0].TryGetLocalPath() : null; + if (string.IsNullOrEmpty(folderPath)) + { + return; + } - try - { await _shell.AddFolderSourceAsync(folderPath); RefreshPanelsFromWorkspace(); } diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml.cs index 09a225c..aa5c2b8 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolView.axaml.cs @@ -59,21 +59,21 @@ private async void OnRequestAddFile(object? sender, EventArgs e) return; } - var files = await topLevel.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions + try { - Title = "Add Workspace File", - AllowMultiple = false, - FileTypeFilter = [SysmlFileType], - }); + var files = await topLevel.StorageProvider.OpenFilePickerAsync(new FilePickerOpenOptions + { + Title = "Add Workspace File", + AllowMultiple = false, + FileTypeFilter = [SysmlFileType], + }); - var filePath = files.Count > 0 ? files[0].TryGetLocalPath() : null; - if (string.IsNullOrEmpty(filePath)) - { - return; - } + var filePath = files.Count > 0 ? files[0].TryGetLocalPath() : null; + if (string.IsNullOrEmpty(filePath)) + { + return; + } - try - { await viewModel.Shell.AddFileSourceAsync(filePath); } catch (Exception ex) @@ -90,20 +90,20 @@ private async void OnRequestAddFolder(object? sender, EventArgs e) return; } - var folders = await topLevel.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions + try { - Title = "Add Workspace Folder", - AllowMultiple = false, - }); + var folders = await topLevel.StorageProvider.OpenFolderPickerAsync(new FolderPickerOpenOptions + { + Title = "Add Workspace Folder", + AllowMultiple = false, + }); - var folderPath = folders.Count > 0 ? folders[0].TryGetLocalPath() : null; - if (string.IsNullOrEmpty(folderPath)) - { - return; - } + var folderPath = folders.Count > 0 ? folders[0].TryGetLocalPath() : null; + if (string.IsNullOrEmpty(folderPath)) + { + return; + } - try - { await viewModel.Shell.AddFolderSourceAsync(folderPath); } catch (Exception ex) From 49fbac24a8a9784a1209843dac319d8fbd20ef23 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 20:12:15 -0400 Subject: [PATCH 11/14] Add legitimate technical terms to cspell dictionary for workspace-view branch --- .cspell.yaml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.cspell.yaml b/.cspell.yaml index 6477d62..c6903d0 100644 --- a/.cspell.yaml +++ b/.cspell.yaml @@ -21,6 +21,8 @@ words: - Dema - doctitle - docversion + - Dedupe + - dedupe - Dockable - Dockables - fileassert @@ -46,8 +48,18 @@ words: - sonar - sonarmark - Stdlib + - Subfolders + - subfolders - sysml - timeslice + - TOCTOU + - unioned + - Unwatch + - unwatch + - Unwatches + - unwatches + - Unwatching + - unwatching - versionmark - Weasyprint - xunit From 14a466621f12497a8c8126c84848abf40f90c9fc Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 20:26:06 -0400 Subject: [PATCH 12/14] Fix case-sensitive path identity for workspace sources Code review finding: WorkspaceSourceSet and WorkspacePanelToolViewModel used OrdinalIgnoreCase for path identity/dedup and folder-grouping comparisons everywhere, which would silently conflate two genuinely distinct files/folders differing only by case on a case-sensitive filesystem (Linux ext4, case-sensitive APFS). - Added WorkspaceSourceSet.NormalizePathCasing: recursively corrects an already-GetFullPath'd path to its actual on-disk casing by walking up to the root and matching each segment case-insensitively against its parent's real directory entries, falling back to the given name if a segment is inaccessible. - AddFile/AddFolder now normalize path casing before dedup, and their existing-source lookups compare with StringComparison.Ordinal instead of OrdinalIgnoreCase. - Resolve()'s fileToSourceId/sourceIdToFiles dictionaries now use StringComparer.Ordinal. - WorkspacePanelToolViewModel.FolderGroup.Subfolders now uses StringComparer.Ordinal, consistent with the normalized, correctly-cased paths it groups. - Cosmetic display-sort OrderBy(..., StringComparer.OrdinalIgnoreCase) calls are left unchanged - alphabetical sort order is a UX concern independent of path identity/dedup correctness. - Added AddFolder_DifferentlyCasedPath_NormalizesToOnDiskCasingAndIsIdempotent test (skips itself on case-sensitive filesystems where the differently cased path does not alias the same folder). - Updated workspace-source-set design doc to document the normalization and its rationale. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .cspell.yaml | 2 + .../workspace-source-set.md | 19 ++++-- .../WorkspacePanelToolViewModel.cs | 2 +- .../WorkspaceSubsystem/WorkspaceSourceSet.cs | 58 +++++++++++++++++-- .../WorkspaceSourceSetTests.cs | 34 +++++++++++ 5 files changed, 104 insertions(+), 11 deletions(-) diff --git a/.cspell.yaml b/.cspell.yaml index c6903d0..beb6fa6 100644 --- a/.cspell.yaml +++ b/.cspell.yaml @@ -11,6 +11,7 @@ version: '0.2' language: en words: + - APFS - Argb - axaml - BGRA @@ -29,6 +30,7 @@ words: - Hmmssfff - LOCALAPPDATA - Mvvm + - ordinally - Pandoc - pagetitle - pasteable diff --git a/docs/design/sysml2-workbench/workspace-subsystem/workspace-source-set.md b/docs/design/sysml2-workbench/workspace-subsystem/workspace-source-set.md index b7a3dcd..03268ed 100644 --- a/docs/design/sysml2-workbench/workspace-subsystem/workspace-source-set.md +++ b/docs/design/sysml2-workbench/workspace-subsystem/workspace-source-set.md @@ -17,8 +17,10 @@ sources currently registered with the workspace. **WorkspaceSource**: `record(string Id, WorkspaceSourceKind Kind, string Path)` — a single registered source. `Id` is a stable `Guid`-derived string that survives reorder/refresh and is used as the file-watcher key and the Workspace -panel's tree-node key. `Path` is absolute and normalized via -`Path.GetFullPath`. +panel's tree-node key. `Path` is absolute (via `Path.GetFullPath`) and further +corrected, segment by segment, to its actual on-disk casing (see `AddFile`/ +`AddFolder`), so every identity/dedupe comparison elsewhere in this unit can +safely use an ordinal (case-sensitive) comparison. **WorkspaceSourceKind**: `enum { File, Folder }` — distinguishes a single-file source from a recursively-globbed folder source. @@ -44,7 +46,11 @@ itself, a `Folder` source maps to its discovered files). `File`-kind source if the exact same normalized path is already registered. - *Preconditions*: None — existence is not enforced at registration time. - *Postconditions*: Idempotent: adding the same normalized path twice returns - the original source rather than creating a duplicate entry. + the original source rather than creating a duplicate entry. Path identity is + compared ordinally (case-sensitively) against each source's already-corrected + on-disk casing, so this is correct on both case-insensitive filesystems + (Windows, default macOS) and case-sensitive ones (Linux, case-sensitive + APFS) alike. **AddFolder**: Registers a folder as a workspace source. @@ -75,7 +81,12 @@ registered source. Files are unioned in source-registration order; on overlap (a file inside a registered folder, or a folder nested inside another registered folder) the first-registered source silently wins attribution in `FileToSourceId` — no - error and no visible "overlap" flag is produced. A zero-source set resolves + error and no visible "overlap" flag is produced. File identity for both + `FileToSourceId` and `SourceIdToFiles` keys is compared ordinally + (case-sensitively), matching each discovered file's actual on-disk casing + (as `GlobFileCollector` and the already-corrected `WorkspaceSource.Path` + both preserve it), so two files differing only by case on a case-sensitive + filesystem are never conflated. A zero-source set resolves to `MergedFiles = []` and empty maps; this is a valid, non-error result. #### Error Handling diff --git a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs index b9f33d4..d1d2c7c 100644 --- a/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs +++ b/src/DemaConsulting.SysML2Workbench/AppShellSubsystem/WorkspacePanelToolViewModel.cs @@ -254,7 +254,7 @@ private sealed class FolderGroup(string RelativePath) { public string RelativePath { get; } = RelativePath; - public Dictionary Subfolders { get; } = new(StringComparer.OrdinalIgnoreCase); + public Dictionary Subfolders { get; } = new(StringComparer.Ordinal); public List Files { get; } = []; } diff --git a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs index 39b57c1..5082410 100644 --- a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs +++ b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/WorkspaceSourceSet.cs @@ -90,9 +90,9 @@ public WorkspaceSource AddFile(string path) { ArgumentException.ThrowIfNullOrWhiteSpace(path); - var normalized = Path.GetFullPath(path); + var normalized = NormalizePathCasing(Path.GetFullPath(path)); var existing = _sources.FirstOrDefault(s => - s.Kind == WorkspaceSourceKind.File && string.Equals(s.Path, normalized, StringComparison.OrdinalIgnoreCase)); + s.Kind == WorkspaceSourceKind.File && string.Equals(s.Path, normalized, StringComparison.Ordinal)); if (existing is not null) { return existing; @@ -119,9 +119,9 @@ public WorkspaceSource AddFolder(string path) throw new DirectoryNotFoundException($"Workspace folder was not found: {path}"); } - var normalized = Path.GetFullPath(path); + var normalized = NormalizePathCasing(Path.GetFullPath(path)); var existing = _sources.FirstOrDefault(s => - s.Kind == WorkspaceSourceKind.Folder && string.Equals(s.Path, normalized, StringComparison.OrdinalIgnoreCase)); + s.Kind == WorkspaceSourceKind.Folder && string.Equals(s.Path, normalized, StringComparison.Ordinal)); if (existing is not null) { return existing; @@ -132,6 +132,52 @@ public WorkspaceSource AddFolder(string path) return source; } + /// + /// Corrects (already 'd) so every + /// segment matches its actual on-disk casing, by walking up to the root and matching each segment + /// case-insensitively against its parent folder's real directory entries. + /// + /// + /// only resolves ./.. segments and relative paths - + /// it does not correct casing, so two paths to the very same on-disk file that were typed, picked, or + /// dropped with different casing (a routine occurrence on case-insensitive filesystems, where the OS + /// happily accepts either) would otherwise be treated as different s. This + /// normalizes once at the source of truth (here, when the source is first added) so every other + /// identity/dedupe comparison throughout can safely use an ordinal + /// (case-sensitive) comparison - which is what actually distinguishes two paths on a case-sensitive + /// filesystem such as Linux ext4, where Foo and foo genuinely are different entries and must + /// not be silently conflated. If a parent segment is inaccessible or vanishes mid-walk, that segment (and + /// everything above it) is left as given rather than failing the whole add. + /// + /// An already-absolute path to normalize. + /// with every segment corrected to its actual on-disk casing. + private static string NormalizePathCasing(string fullPath) + { + var parent = Path.GetDirectoryName(fullPath); + var name = Path.GetFileName(fullPath); + + if (string.IsNullOrEmpty(parent) || string.IsNullOrEmpty(name)) + { + // Reached a root (for example "C:\" or "/") - nothing further above it to normalize. + return fullPath; + } + + var normalizedParent = NormalizePathCasing(parent); + + try + { + var actualName = Directory.EnumerateFileSystemEntries(normalizedParent) + .Select(Path.GetFileName) + .FirstOrDefault(entry => string.Equals(entry, name, StringComparison.OrdinalIgnoreCase)); + + return Path.Combine(normalizedParent, actualName ?? name); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + return Path.Combine(normalizedParent, name); + } + } + /// /// Removes the source with the given identifier, if one is registered. /// @@ -165,8 +211,8 @@ public bool RemoveSource(string sourceId) public WorkspaceSourceResolution Resolve() { var mergedFiles = new List(); - var fileToSourceId = new Dictionary(StringComparer.OrdinalIgnoreCase); - var sourceIdToFiles = new Dictionary>(StringComparer.OrdinalIgnoreCase); + var fileToSourceId = new Dictionary(StringComparer.Ordinal); + var sourceIdToFiles = new Dictionary>(StringComparer.Ordinal); foreach (var source in _sources) { diff --git a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs index 5066111..41d8bfb 100644 --- a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs +++ b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/WorkspaceSourceSetTests.cs @@ -195,6 +195,40 @@ public async Task Resolve_NestedFolderOverlap_DedupesAndFirstRegisteredSourceWin Assert.Equal(outerSource.Id, resolution.FileToSourceId[resolution.MergedFiles[0]]); } + /// + /// Validates that corrects a differently-cased path to its + /// actual on-disk casing, and that the corrected casing is what makes adding the same folder again + /// (via yet another differently-cased path) idempotent rather than registering a duplicate source - + /// the underlying scenario is two case-distinct paths to the very same on-disk folder, which routinely + /// happens on case-insensitive filesystems (Windows, default macOS) when a path is typed, drag-dropped, + /// or picked with different casing than the real directory entry. + /// + [Fact] + public void AddFolder_DifferentlyCasedPath_NormalizesToOnDiskCasingAndIsIdempotent() + { + // Arrange + var actualName = Path.GetFileName(_tempRoot); + var upperCasedPath = Path.Combine(Path.GetDirectoryName(_tempRoot)!, actualName.ToUpperInvariant()); + if (!Directory.Exists(upperCasedPath)) + { + // The host filesystem is case-sensitive (for example Linux ext4), so the differently-cased path + // genuinely does not resolve to the same directory - the normalization behavior under test only + // applies on case-insensitive filesystems (Windows, default macOS). + Assert.Skip("Host filesystem is case-sensitive; differently-cased path does not alias the same folder."); + } + + var sourceSet = new WorkspaceSourceSet(); + + // Act + var first = sourceSet.AddFolder(_tempRoot); + var second = sourceSet.AddFolder(upperCasedPath); + + // Assert: both additions resolve to the same source, and its Path reflects the real on-disk casing + Assert.Equal(first.Id, second.Id); + Assert.Single(sourceSet.Sources); + Assert.Equal(_tempRoot, first.Path, StringComparer.Ordinal); + } + /// /// Validates that preserves registration order across mixed /// file and folder additions. From 9b2562723a7a2d9a1685b8002e28338562bf1570 Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 20:43:07 -0400 Subject: [PATCH 13/14] Fix pre-existing FileWatcher thread-safety race causing CI crashes The Windows leg of CI was intermittently crashing the whole test host with a FATAL ERROR / catastrophic failure: 'Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state.' Root cause: FileSystemWatcher raises its Changed/Created/Deleted/Renamed events on an operating-system callback thread. Production code marshals those callbacks onto the UI thread first, but ImmediateUiDispatcher (used by default outside a live UI, including by unit tests that exercise a real FileSystemWatcher) runs them synchronously on that OS callback thread instead. FileWatcher's internal _pending and _watchersBySourceId dictionaries were mutated/enumerated from that background thread with no synchronization against the calling thread (WatchSource/UnwatchSource/QueueChange/FlushPendingChanges/ PendingChanges), so a callback racing a caller could corrupt a Dictionary's internal state - not merely throw a recoverable exception - and crash the process outright. This was pre-existing, undetected until now because CI's Windows runner apparently delivers FileSystemWatcher callbacks promptly/reliably enough to hit the race, unlike ubuntu-latest/ macos-latest in this run. - Added an internal Lock (_gate) guarding every read/write of _pending and _watchersBySourceId across WatchSource, UnwatchSource, QueueChange, FlushPendingChanges, PendingChanges, WatchedSourceIds, and Dispose. - WatchSource now registers the new watcher and captures any previous watcher for the same source id inside the lock, then enables events and disposes the previous watcher outside the lock - so a racing callback never observes a moment where the source id is unwatched, and disposal never happens while holding the lock. - Updated the stale 'not thread-safe, tolerant of transient enumeration exceptions' comment/workaround in the existing real-FileSystemWatcher test, since that tolerance is no longer needed. - Added ConcurrentRealFileSystemChangesAndReads_DoesNotCorruptState: drives a real FileSystemWatcher under sustained concurrent writes for 2 seconds while continuously reading PendingChanges/ FlushPendingChanges from the test thread, asserting no exception surfaces from either side. - Updated the file-watcher design doc's Error Handling section to document the callback-thread hazard and the locking guarantee. Verified stable across repeated local runs of the affected test file and a new sustained-concurrency stress test; full suite 154/154 passing, lint.ps1 clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .cspell.yaml | 1 + .../workspace-subsystem/file-watcher.md | 11 ++ .../WorkspaceSubsystem/FileWatcher.cs | 122 +++++++++++++----- .../WorkspaceSubsystem/FileWatcherTests.cs | 68 ++++++++-- 4 files changed, 157 insertions(+), 45 deletions(-) diff --git a/.cspell.yaml b/.cspell.yaml index beb6fa6..bd9e83a 100644 --- a/.cspell.yaml +++ b/.cspell.yaml @@ -55,6 +55,7 @@ words: - sysml - timeslice - TOCTOU + - uncontended - unioned - Unwatch - unwatch diff --git a/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md b/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md index d759b38..4601bf7 100644 --- a/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md +++ b/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md @@ -85,6 +85,17 @@ boundary - each `FileSystemWatcher` only reports events from its own root (and, for file sources, its own `Filter`). Logging of watcher faults is best-effort and does not replace propagation to the shell. +Every `FileSystemWatcher` raises its `Changed`/`Created`/`Deleted`/`Renamed` +events on an operating-system callback thread, independent of whatever thread +calls `WatchSource`/`UnwatchSource`/`FlushPendingChanges`/`PendingChanges`. +Production usage marshals those callbacks onto the UI thread first, making +this uncontended in practice, but a caller that supplies (or defaults to) an +immediate, non-marshaling dispatcher runs them synchronously on the OS +callback thread itself. `FileWatcher` therefore guards all reads and writes of +its internal pending-changes and watcher-registry state with a single +internal lock, so a change notification racing a concurrent call from another +thread can never corrupt that state. + #### Dependencies - **WorkspaceModel** — applies the incremental reload once a stable change batch diff --git a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs index 5f8bd34..a2728c7 100644 --- a/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs +++ b/src/DemaConsulting.SysML2Workbench/WorkspaceSubsystem/FileWatcher.cs @@ -42,6 +42,20 @@ public void Post(Action action) /// public sealed class FileWatcher : IDisposable { + /// + /// Guards and , both of which are read and + /// written from the calling thread (via // + /// /) as well as from whatever background + /// thread the operating system delivers a callback on. Production usage + /// marshals those callbacks onto the UI thread via , making this uncontended in + /// practice, but (used by default outside a live UI, including in + /// tests exercising a real ) runs them synchronously on the OS callback + /// thread itself - without this lock, that thread and the calling thread can mutate/enumerate the same + /// concurrently, which can corrupt its internal state and crash the + /// process rather than merely throwing a recoverable exception. + /// + private readonly Lock _gate = new(); + /// /// Normalized paths queued for the next incremental refresh cycle, mapped to the timestamp of their most /// recent change notification. @@ -80,7 +94,16 @@ public FileWatcher(TimeSpan debounceWindow, Func? clock = null, /// /// Identifiers of every source currently being monitored for external changes. /// - public IReadOnlySet WatchedSourceIds => _watchersBySourceId.Keys.ToHashSet(StringComparer.Ordinal); + public IReadOnlySet WatchedSourceIds + { + get + { + lock (_gate) + { + return _watchersBySourceId.Keys.ToHashSet(StringComparer.Ordinal); + } + } + } /// /// Minimum delay used to merge rapid change bursts into a single reload batch. @@ -90,7 +113,16 @@ public FileWatcher(TimeSpan debounceWindow, Func? clock = null, /// /// Normalized paths queued for the next incremental refresh cycle. /// - public IReadOnlySet PendingChanges => new HashSet(_pending.Keys, StringComparer.OrdinalIgnoreCase); + public IReadOnlySet PendingChanges + { + get + { + lock (_gate) + { + return new HashSet(_pending.Keys, StringComparer.OrdinalIgnoreCase); + } + } + } /// /// Begins monitoring the given source. Calling this again for a source whose id is already watched @@ -109,11 +141,6 @@ public void WatchSource(WorkspaceSource source) { ArgumentNullException.ThrowIfNull(source); - if (_watchersBySourceId.Remove(source.Id, out var previous)) - { - previous.Dispose(); - } - var watcher = source.Kind == WorkspaceSourceKind.Folder ? CreateFolderWatcher(source.Path) : CreateFileWatcher(source.Path); @@ -122,9 +149,23 @@ public void WatchSource(WorkspaceSource source) watcher.Created += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); watcher.Deleted += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); watcher.Renamed += (_, e) => _dispatcher.Post(() => QueueChange(e.FullPath)); - watcher.EnableRaisingEvents = true; - _watchersBySourceId[source.Id] = watcher; + FileSystemWatcher? previous = null; + lock (_gate) + { + if (_watchersBySourceId.Remove(source.Id, out var existing)) + { + previous = existing; + } + + _watchersBySourceId[source.Id] = watcher; + } + + // Enabled outside the lock, and any previously registered watcher for this source id is disposed only + // after the new one has fully taken its place, so a callback racing this method never observes a gap + // where the source id is momentarily unwatched. + watcher.EnableRaisingEvents = true; + previous?.Dispose(); } /// @@ -139,9 +180,13 @@ public void WatchSource(WorkspaceSource source) /// when a watcher was found and removed; otherwise . public bool UnwatchSource(string sourceId) { - if (!_watchersBySourceId.Remove(sourceId, out var watcher)) + FileSystemWatcher? watcher; + lock (_gate) { - return false; + if (!_watchersBySourceId.Remove(sourceId, out watcher)) + { + return false; + } } watcher.Dispose(); @@ -208,14 +253,17 @@ private static FileSystemWatcher CreateFileWatcher(string filePath) public void QueueChange(string path) { ArgumentException.ThrowIfNullOrWhiteSpace(path); - if (_watchersBySourceId.Count == 0) + lock (_gate) { - return; - } + if (_watchersBySourceId.Count == 0) + { + return; + } - // Overwriting the timestamp for an already-pending path is what collapses bursty duplicate - // notifications into a single pending entry while still resetting its debounce window - _pending[path] = _clock(); + // Overwriting the timestamp for an already-pending path is what collapses bursty duplicate + // notifications into a single pending entry while still resetting its debounce window + _pending[path] = _clock(); + } } /// @@ -230,33 +278,41 @@ public void QueueChange(string path) /// Normalized paths requiring reload. public IReadOnlyList FlushPendingChanges() { - if (_watchersBySourceId.Count == 0) + lock (_gate) { - return []; - } + if (_watchersBySourceId.Count == 0) + { + return []; + } - var now = _clock(); - var ready = _pending - .Where(kvp => now - kvp.Value >= DebounceWindow) - .Select(kvp => kvp.Key) - .ToList(); + var now = _clock(); + var ready = _pending + .Where(kvp => now - kvp.Value >= DebounceWindow) + .Select(kvp => kvp.Key) + .ToList(); - foreach (var path in ready) - { - _pending.Remove(path); - } + foreach (var path in ready) + { + _pending.Remove(path); + } - return ready; + return ready; + } } /// public void Dispose() { - foreach (var watcher in _watchersBySourceId.Values) + List watchers; + lock (_gate) { - watcher.Dispose(); + watchers = [.. _watchersBySourceId.Values]; + _watchersBySourceId.Clear(); } - _watchersBySourceId.Clear(); + foreach (var watcher in watchers) + { + watcher.Dispose(); + } } } diff --git a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs index 14c9bf4..29c91e5 100644 --- a/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs +++ b/test/DemaConsulting.SysML2Workbench.Tests/WorkspaceSubsystem/FileWatcherTests.cs @@ -297,22 +297,14 @@ public async Task WatchSource_TwoFolders_ChangeUnderOneIsNotAttributedToTheOther var changedPathUnderB = Path.Combine(rootB, "Model.sysml"); await File.WriteAllTextAsync(changedPathUnderB, "part def Vehicle;", TestContext.Current.CancellationToken); - // Poll with a bounded timeout: real FileSystemWatcher notifications are asynchronous, and the - // underlying pending dictionary is not thread-safe against the watcher's own OS-callback thread, so - // reads are tolerant of transient enumeration exceptions. + // Poll with a bounded timeout: real FileSystemWatcher notifications are delivered on an OS callback + // thread, asynchronously and non-deterministically with respect to this polling loop. var deadline = DateTime.UtcNow.AddSeconds(5); var detected = false; while (DateTime.UtcNow < deadline && !detected) { - try - { - detected = watcher.PendingChanges.Any(path => - string.Equals(path, changedPathUnderB, StringComparison.OrdinalIgnoreCase)); - } - catch - { - // Transient enumeration failure racing the watcher's own background callback - retry. - } + detected = watcher.PendingChanges.Any(path => + string.Equals(path, changedPathUnderB, StringComparison.OrdinalIgnoreCase)); if (!detected) { @@ -330,4 +322,56 @@ public async Task WatchSource_TwoFolders_ChangeUnderOneIsNotAttributedToTheOther Directory.Delete(rootB, recursive: true); } } + + /// + /// Regression test: reproduces many real, concurrently-arriving + /// callbacks (via , which runs them synchronously on their own OS + /// callback thread rather than marshaling onto a UI thread) racing repeated reads via + /// and from the + /// test thread. Previously, concurrent unsynchronized access to the internal pending-changes dictionary + /// could corrupt its state and crash the whole process rather than merely throwing a recoverable + /// exception; this must now complete cleanly under sustained concurrent load. + /// + [Fact] + public async Task ConcurrentRealFileSystemChangesAndReads_DoesNotCorruptState() + { + // Arrange: a real, immediately-dispatched watcher so OS callbacks run on background threads + var watcher = new FileWatcher(TimeSpan.FromMilliseconds(10)); + watcher.WatchSource(FolderSource(_tempRoot)); + + // Act: concurrently generate a burst of real file-system events while continuously reading and + // flushing from the test thread, for a short but sustained window. + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(2)); + var writerTask = Task.Run(async () => + { + var counter = 0; + while (!cts.IsCancellationRequested) + { + var path = Path.Combine(_tempRoot, $"File{counter++ % 20}.sysml"); + try + { + await File.WriteAllTextAsync(path, "part def Widget;", CancellationToken.None); + } + catch (IOException) + { + // Transient sharing violation writing the same rotating file names under load - benign. + } + } + }, CancellationToken.None); + + var exception = await Record.ExceptionAsync(async () => + { + while (!cts.IsCancellationRequested) + { + _ = watcher.PendingChanges.Count; + _ = watcher.FlushPendingChanges(); + await Task.Delay(5, CancellationToken.None); + } + }); + + await writerTask; + + // Assert: no exception surfaced from either the reader or writer side under concurrent load + Assert.Null(exception); + } } From b9e97f76da24ed7a418707bde8a1ceb4631537cf Mon Sep 17 00:00:00 2001 From: Malcolm Nixon Date: Sun, 19 Jul 2026 21:03:36 -0400 Subject: [PATCH 14/14] Fix stale requirement/verification docs for FileWatcher empty-watch behavior CI's requirements enforcement failed: 'Only 75 of 76 requirements are satisfied with tests. Unsatisfied requirements: SysML2Workbench-WorkspaceSubsystem-FileWatcher-RequireWatchBeforeQueue' The requirement and its verification doc still referenced QueueChange_BeforeWatchSource_ThrowsInvalidOperationException, a test that no longer exists - FileWatcher.QueueChange's behavior was changed in an earlier commit this session (supporting an empty workspace as a first-class state) from throwing InvalidOperationException to silently ignoring the notification, and was covered by a replacement test, QueueChange_WithNoWatchedSources_IsIgnored, but the reqstream requirement/verification docs were never updated to match, so reqstream's --enforce trx cross-check (which dotnet reqstream --lint does not perform) correctly flagged the now-nonexistent referenced test. - Updated the RequireWatchBeforeQueue requirement's title, justification, and referenced test to reflect the actual silently-ignored behavior. - Updated the corresponding verification doc entry. - Corrected the design doc's QueueChange/FlushPendingChanges preconditions, which described the old hard 'at least one source watched' requirement rather than the current valid-empty-state behavior. Verified locally by reproducing the CI trx-enforcement check (dotnet test --logger trx + dotnet reqstream --enforce): the RequireWatchBeforeQueue requirement is now satisfied. lint.ps1 clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../workspace-subsystem/file-watcher.md | 14 +++++++++----- .../workspace-subsystem/file-watcher.yaml | 6 +++--- .../workspace-subsystem/file-watcher.md | 7 ++++--- 3 files changed, 16 insertions(+), 11 deletions(-) diff --git a/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md b/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md index 4601bf7..a71ccf1 100644 --- a/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md +++ b/docs/design/sysml2-workbench/workspace-subsystem/file-watcher.md @@ -58,10 +58,13 @@ already-watched source with the same id if one exists. - *Parameters*: `string path` — file or folder path reported by the platform. - *Returns*: `void` — the change is added to the pending set. -- *Preconditions*: At least one source is currently watched. -- *Postconditions*: Duplicate or bursty notifications for the same path - collapse into a single pending change entry, regardless of which watched - source's watcher raised the underlying event. +- *Preconditions*: None - zero watched sources (an empty workspace, or a stale + notification racing an unwatch) is a valid, first-class state. +- *Postconditions*: When at least one source is watched, duplicate or bursty + notifications for the same path collapse into a single pending change + entry, regardless of which watched source's watcher raised the underlying + event. When no source is watched, the notification is silently ignored and + `PendingChanges` is left unchanged. **FlushPendingChanges**: Dispatches the current batch to the workspace reload pipeline. @@ -69,7 +72,8 @@ pipeline. - *Parameters*: `None` — operates on the accumulated pending changes. - *Returns*: `IReadOnlyList` — normalized paths requiring reload, merged across every watcher that reported a change since the last flush. -- *Preconditions*: Monitoring has started for at least one source. +- *Preconditions*: None - zero watched sources is a valid state and simply + yields an empty result. - *Postconditions*: Returned paths are removed from `PendingChanges` and are ready for `WorkspaceModel.ReloadFiles`. diff --git a/docs/reqstream/sysml2-workbench/workspace-subsystem/file-watcher.yaml b/docs/reqstream/sysml2-workbench/workspace-subsystem/file-watcher.yaml index 60c35a7..8e441ff 100644 --- a/docs/reqstream/sysml2-workbench/workspace-subsystem/file-watcher.yaml +++ b/docs/reqstream/sysml2-workbench/workspace-subsystem/file-watcher.yaml @@ -39,8 +39,8 @@ sections: - 'UnwatchSource_UnknownSourceId_ReturnsFalse' - id: 'SysML2Workbench-WorkspaceSubsystem-FileWatcher-RequireWatchBeforeQueue' - title: 'The FileWatcher shall reject queuing a change before any source is being watched.' + title: 'The FileWatcher shall silently ignore queuing a change when no source is currently being watched.' justification: | - Queuing changes with no active watcher would silently accumulate pending state that can never be attributed to a real source. + Zero watched sources is a first-class, valid empty-workspace state, and a change notification can legitimately race an unwatch; queuing must not accumulate unattributable pending state or throw. tests: - - 'QueueChange_BeforeWatchSource_ThrowsInvalidOperationException' + - 'QueueChange_WithNoWatchedSources_IsIgnored' diff --git a/docs/verification/sysml2-workbench/workspace-subsystem/file-watcher.md b/docs/verification/sysml2-workbench/workspace-subsystem/file-watcher.md index 1d8d6dc..33db548 100644 --- a/docs/verification/sysml2-workbench/workspace-subsystem/file-watcher.md +++ b/docs/verification/sysml2-workbench/workspace-subsystem/file-watcher.md @@ -60,6 +60,7 @@ their previously queued pending state survives. Verified by **UnwatchSource_UnknownSourceId_ReturnsFalse**: Unwatching an id that is not currently watched returns `false` without throwing. Verified by `FileWatcherTests.UnwatchSource_UnknownSourceId_ReturnsFalse`. -**QueueChange_BeforeWatchSource_ThrowsInvalidOperationException**: Queuing a change before any source has been -watched throws `InvalidOperationException` rather than silently accumulating unattributable pending state. Verified -by `FileWatcherTests.QueueChange_BeforeWatchSource_ThrowsInvalidOperationException`. +**QueueChange_WithNoWatchedSources_IsIgnored**: Queuing a change when no source is currently watched (before any +source has been watched, or after the last one has been unwatched) is silently ignored rather than throwing or +accumulating unattributable pending state, since zero watched sources is a first-class, valid empty-workspace +state. Verified by `FileWatcherTests.QueueChange_WithNoWatchedSources_IsIgnored`.