Implement multi-source workspace with enhanced UI and file handling - #4
Merged
Conversation
…spaceSourceSet 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>
…mpty-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>
…ace and Workspace panel Adds design docs, ReqStream requirements, verification docs, and SysML2 model entries for the two new Units introduced by the workspace-view feature: WorkspaceSourceSet (WorkspaceSubsystem) and WorkspacePanel (AppShellSubsystem, covering WorkspacePanelToolViewModel/View together). Rewrites the affected existing companion docs to describe the new multi-source model instead of a single root path: - workspace-subsystem.md / workspace-subsystem.sysml (contains-list, boundary language, per-source watch scope in the Design steps) - workspace-model.md / workspace-model.yaml / workspace-model.md (verification) - Sources replaces RootPath, drops the stale unimplemented ImportGraph data-model entry, documents the optional updatedResolution parameter on ReloadFiles and empty-resolution support - file-watcher.md / file-watcher.yaml / file-watcher.md (verification) - WatchSource/UnwatchSource replace StartWatching, documents per-source isolation - app-shell-subsystem.md - adds the Workspace panel and the "zero sources is a first-class state" contract across all four panels - main-window-shell.md / main-window-shell.yaml / main-window-shell.md (verification) - AddFileSource/AddFolderSource/RemoveSource replace OpenWorkspace, documents eager empty-snapshot-at-construction and the new SourcesChanged notification Adds the two new reqstream files to requirements.yaml's includes list and two new review-set entries to .reviewmark.yaml (WorkspaceSourceSet under the existing WorkspaceSubsystem context, WorkspacePanel under the existing AppShellSubsystem context). Updates docs/user_guide/getting_started.md and introduction.md to describe building a workspace from multiple file/folder sources, the new Workspace panel, drag-and-drop, and the empty-workspace state, instead of a single "Open Workspace" folder picker. Regenerated docs/design/generated/*.svg locally via `sysml2tools render` for validation; that folder is gitignored and not part of this commit. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…bugs - Add a .sysml FilePickerFileType to both "Open File" pickers (main window File menu and the Workspace panel's Add File button) - the picker previously showed all files with no filter. - Add ProportionalDockSplitter dockables between every pane in WorkbenchDockFactory's main horizontal and center-vertical ProportionalDocks. Without an explicit splitter dockable between entries, AvaloniaUI/Dock renders no drag handle at all between panes, so none of the tool panels (including the new Workspace panel) could be resized by dragging - only via double-click-to-auto-size or none at all. This was missed when the dock layout was first composed. - FileWatcher.QueueChange/FlushPendingChanges no longer throw InvalidOperationException when zero sources are currently watched. A source's OS watcher callback is marshalled onto the dispatcher asynchronously, so a change notification can arrive after the owning source was unwatched (most commonly: the user removes the last remaining workspace source while a notification for it is already queued). Since zero watched sources is now a first-class supported state (an empty workspace), this is a normal race, not misuse - it is now silently ignored instead of throwing unhandled on the dispatcher. Added a regression test reproducing the exact race (QueueChange_AfterSourceUnwatchedToZero_DoesNotThrow) and updated the existing "before any source is watched" test to match the new contract. - WorkspacePanelToolView's Add File/Add Folder/drag-drop handlers now wrap their calls to MainWindowShell.AddFileSourceAsync/ AddFolderSourceAsync in try/catch, surfacing failures via StatusMessage - matching the equivalent MainWindowView handlers, which already did this. Previously an exception from these async void handlers (e.g. a dropped/picked folder deleted between selection and the call, or a permissions error) would go unhandled and crash the app, unlike the functionally identical File-menu/main-window-drop path. The dispatcher-race and inconsistent-exception-handling issues were found by an automated code-review pass over this branch; the missing file-type filter and missing dock splitters were found through manual local testing. Verified: dotnet build (0 warnings/errors), dotnet test (151/151, up from 150 with the new regression test), dotnet sysml2tools lint (clean), dotnet reqstream --lint (clean), pwsh ./fix.ps1 (clean, git diff --stat confirms only the 5 intentionally-touched files changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CustomViewBuilderToolView's root was a plain StackPanel with no enclosing ScrollViewer, unlike the Diagnostics/PredefinedViews/Workspace panels (which all end in a self-scrolling ListBox/TreeView fill element). On smaller windows/DPI settings its content (view kind, expose targets, filter expression, action buttons, status) can exceed the panel's available height with no way to reach the rest - the content silently clips instead of scrolling. Wrapped the root StackPanel in a ScrollViewer so overflow content becomes reachable via a vertical scrollbar. Verified the sibling panels (Diagnostics, PredefinedViews, Workspace) do not have this issue - each already ends in a ListBox/TreeView, which provides its own scrolling automatically. Verified: dotnet build (0 errors), dotnet test (151/151), pwsh ./fix.ps1 (clean, only this file changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The Workspace panel's tree rendered every node (folder sources, file sources, and individual files) as plain text with no icon, unlike a typical file tree. Added the Material.Icons.Avalonia package (chosen over hand-authored geometry icons since we expect to need icons more broadly soon, e.g. an upcoming toolbar and menu items - the package's MaterialIcon control drops into any icon slot: tree items, MenuItem.Icon, button content, etc.). - Added WorkspaceTreeNode.IconKind (abstract), overridden per node type: WorkspaceSourceNode picks Folder vs FileOutline based on Source.Kind, WorkspaceFileNode always uses FileOutline. - Updated WorkspacePanelToolView's TreeDataTemplates to render a small MaterialIcon beside each node's text. Verified: dotnet build (0 warnings/errors), dotnet test (151/151), pwsh ./fix.ps1 (clean, only the 3 intended files changed). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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>
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 <materialIcons:MaterialIconStyles /> 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>
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<WorkspaceTreeNode> (was IReadOnlyList<WorkspaceFileNode>): 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>
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>
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>
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>
…ehavior 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request expands the SysML2 Workbench's documentation and review coverage to include the new WorkspacePanel and WorkspaceSourceSet units, and updates the AppShellSubsystem and WorkspaceSubsystem designs to reflect support for multiple file and folder sources, a first-class empty workspace state, and improved UI behaviors. It also adds new review definitions and cspell dictionary entries for relevant terminology.
Documentation and Design Updates:
docs/design/sysml2-workbench/app-shell-subsystem.md,docs/design/sysml2-workbench/app-shell-subsystem/main-window-shell.md,docs/design/sysml2-workbench/workspace-subsystem.md: Updated to document the WorkspacePanel as a new tool panel, clarify that the workspace can have zero or more sources (files or folders), and describe the new empty-state behaviors and source management methods (AddFileSource,AddFolderSource,RemoveSource). Also, the WorkspaceSubsystem's boundary is now defined as an ordered set of user-added sources, not just a root folder. [1] [2] [3] [4] [5] [6] [7]docs/design/sysml2-workbench/app-shell-subsystem/workspace-panel.md: Added detailed documentation for the new WorkspacePanel, including its data model, tree structure, commands, error handling, and integration with the shell and UI.Review Coverage:
.reviewmark.yaml: Added review definitions for WorkspaceSourceSet and WorkspacePanel, specifying their implementation files and associated design/requirement documents, ensuring these units are included in the formal review process. [1] [2]Spelling Dictionary:
.cspell.yaml: Added cspell entries for new terminology such as "Dedupe", "Subfolders", "Unwatch", and others relevant to the new features and documentation. [1] [2] [3]