Browse and compare below the top folder, and keep Give/Take/X failures out of the render loop [patch]

CLAUDE.md

@@ -26,7 +26,7 @@ Tests live in `ProjectDirector.Test` (MSTest, via `MSTest.Sdk` + `ktsu.Sdk`). Th
 
 `tests/ProjectDirector.UITests` drives the real application headlessly through `ktsu.ImGui.App.Testing` to render the app gallery (`Gallery/`), and exists for that purpose. It reaches the app through `ProjectDirector.BuildConfig()` and the internal options constructor. It replaces the settings file and the secret store with in-memory ones, and points the app at three repositories it seeds offline under `/tmp/projectdirector-gallery`. That path is fixed because it appears in the pictures. Every picture is cropped above the log panel, because the log prints wall-clock timestamps. `PROJECTDIRECTOR_GALLERY_OUT` names the output directory; `.github/workflows/app-gallery.yml` regenerates `docs/gallery/` after CI passes on `main`. A new view or dialog worth showing gets an entry in `Gallery/GalleryCatalog.cs`.
 
-That last point is why the repository actions are shaped the way they are: the part of each with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display. `ProjectDirector.DecidePull` decides whether pulling interrupts the user first (`PullDecisionTests`), and `GitCli.ListPendingChanges` plus `ProjectDirector.DescribePendingChanges` decide what a commit will sweep up and how that is shown (`CommitTests`). `DiffTake` builds the file a take arrow in the diff view produces and writes it back with the destination's own line ending and BOM (`DiffTakeTests`). Anything genuinely worth testing that is still tangled up with drawing is usually worth extracting the same way.
+That last point is why the repository actions are shaped the way they are: the part of each with a rule in it is pulled out into a plain method so it can be driven without a live ImGui context or a display. `ProjectDirector.DecidePull` decides whether pulling interrupts the user first (`PullDecisionTests`), and `GitCli.ListPendingChanges` plus `ProjectDirector.DescribePendingChanges` decide what a commit will sweep up and how that is shown (`CommitTests`). `RepoBrowsing` holds the repo and compare browsers' path handling: every entry is relative to the repository root and already includes the browse path, and folders are recognised on disk rather than by a trailing separator (`RepoBrowsingTests`). `DiffTake` builds the file a take arrow in the diff view produces and writes it back with the destination's own line ending and BOM (`DiffTakeTests`). Anything genuinely worth testing that is still tangled up with drawing is usually worth extracting the same way.
 
 ```powershell
 dotnet test --configuration Release

ProjectDirector.Test/RepoBrowsingTests.cs

@@ -0,0 +1,166 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.ProjectDirector.Test;
+
+using System;
+using System.Collections.ObjectModel;
+using System.IO;
+using System.Linq;
+
+using ktsu.Semantics.Paths;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Tests the path handling behind the repo and compare browsers below the top level.
+/// </summary>
+/// <remarks>
+/// Entries used to be combined with the browse path a second time, so opening <c>src</c> looked for
+/// <c>src/src/Inner</c>: folders showed as files, navigation stopped at the first level, Give on a
+/// file threw on the render thread, and Give on a folder created a stray <c>src/src/...</c> in the
+/// other repository. Folders were also recognised by a trailing separator that
+/// <see cref="RelativePath"/> strips, so the compare browser called <see cref="File.Copy(string, string)"/>
+/// and <see cref="File.Delete(string)"/> on folders.
+/// </remarks>
+[TestClass]
+public sealed class RepoBrowsingTests
+{
+	private string _workspace = string.Empty;
+	private string _repoA = string.Empty;
+	private string _repoB = string.Empty;
+
+	[TestInitialize]
+	public void CreateRepos()
+	{
+		_workspace = Path.Join(Path.GetTempPath(), $"ktsu_pd_browse_{Guid.NewGuid():N}");
+		_repoA = Path.Join(_workspace, "A");
+		_repoB = Path.Join(_workspace, "B");
+
+		_ = Directory.CreateDirectory(Path.Join(_repoA, "src", "Inner"));
+		File.WriteAllText(Path.Join(_repoA, "src", "a.cs"), "class A { }\n");
+		_ = Directory.CreateDirectory(Path.Join(_repoA, "only"));
+		_ = Directory.CreateDirectory(Path.Join(_repoB, "src"));
+	}
+
+	[TestCleanup]
+	public void DeleteRepos()
+	{
+		try
+		{
+			Directory.Delete(_workspace, recursive: true);
+		}
+		catch (IOException)
+		{
+			// A leaked temp directory is not worth failing an otherwise passing test over.
+		}
+	}
+
+	private static RelativePath Entry(string path) => RelativePath.Create<RelativePath>(path);
+
+	[TestMethod]
+	public void List_ANestedFolder_ReturnsEntriesRelativeToTheRepoRoot()
+	{
+		Collection<RelativePath> entries = RepoBrowsing.List(_repoA, "src");
+
+		CollectionAssert.AreEquivalent(
+			new[] { Entry(Path.Join("src", "Inner")), Entry(Path.Join("src", "a.cs")) },
+			entries.ToArray());
+	}
+
+	[TestMethod]
+	public void List_AFolderTheRepoDoesNotHave_ReturnsNothing()
+	{
+		Assert.IsEmpty(RepoBrowsing.List(_repoB, "only"));
+	}
+
+	[TestMethod]
+	public void IsDirectory_ANestedFolder_IsAFolder()
+	{
+		Collection<RelativePath> entries = RepoBrowsing.List(_repoA, "src");
+
+		RelativePath[] directories = [.. entries.Where(entry => RepoBrowsing.IsDirectory(entry, _repoA, _repoB))];
+
+		CollectionAssert.AreEqual(new[] { Entry(Path.Join("src", "Inner")) }, directories);
+	}
+
+	[TestMethod]
+	public void IsDirectory_AFolderInOnlyOneRepo_IsAFolder()
+	{
+		Assert.IsTrue(RepoBrowsing.IsDirectory(Entry("only"), _repoB, _repoA));
+	}
+
+	[TestMethod]
+	public void Navigating_IntoANestedFolder_ListsItsContents()
+	{
+		File.WriteAllText(Path.Join(_repoA, "src", "Inner", "b.cs"), "class B { }\n");
+		RelativePath inner = Entry(Path.Join("src", "Inner"));
+
+		Collection<RelativePath> entries = RepoBrowsing.List(_repoA, inner.WeakString);
+
+		CollectionAssert.AreEqual(new[] { Entry(Path.Join("src", "Inner", "b.cs")) }, entries.ToArray());
+	}
+
+	[TestMethod]
+	public void Copy_ANestedFile_LandsAtTheSamePathInTheOtherRepo()
+	{
+		string? failure = RepoBrowsing.Copy(Entry(Path.Join("src", "a.cs")), _repoA, _repoB);
+
+		Assert.IsNull(failure);
+		Assert.IsTrue(File.Exists(Path.Join(_repoB, "src", "a.cs")));
+		Assert.IsFalse(Directory.Exists(Path.Join(_repoB, "src", "src")));
+	}
+
+	[TestMethod]
+	public void Copy_ANestedFolder_CreatesItWithoutAStrayParent()
+	{
+		string? failure = RepoBrowsing.Copy(Entry(Path.Join("src", "Inner")), _repoA, _repoB);
+
+		Assert.IsNull(failure);
+		Assert.IsTrue(Directory.Exists(Path.Join(_repoB, "src", "Inner")));
+		Assert.IsFalse(Directory.Exists(Path.Join(_repoB, "src", "src")));
+	}
+
+	[TestMethod]
+	public void Copy_AFolderInOnlyOneRepo_CreatesTheFolderRatherThanThrowing()
+	{
+		string? failure = RepoBrowsing.Copy(Entry("only"), _repoA, _repoB);
+
+		Assert.IsNull(failure);
+		Assert.IsTrue(Directory.Exists(Path.Join(_repoB, "only")));
+	}
+
+	[TestMethod]
+	public void Copy_ASourceThatIsGone_ReportsTheFailureRatherThanThrowing()
+	{
+		string? failure = RepoBrowsing.Copy(Entry(Path.Join("src", "missing.cs")), _repoA, _repoB);
+
+		Assert.IsNotNull(failure);
+	}
+
+	[TestMethod]
+	public void Delete_AFolder_RemovesItRatherThanThrowing()
+	{
+		string? failure = RepoBrowsing.Delete(Entry("only"), _repoA);
+
+		Assert.IsNull(failure);
+		Assert.IsFalse(Directory.Exists(Path.Join(_repoA, "only")));
+	}
+
+	[TestMethod]
+	public void Delete_ANestedFile_RemovesOnlyThatFile()
+	{
+		string? failure = RepoBrowsing.Delete(Entry(Path.Join("src", "a.cs")), _repoA);
+
+		Assert.IsNull(failure);
+		Assert.IsFalse(File.Exists(Path.Join(_repoA, "src", "a.cs")));
+		Assert.IsTrue(Directory.Exists(Path.Join(_repoA, "src", "Inner")));
+	}
+
+	[TestMethod]
+	public void Delete_AFolderWithContents_ReportsTheFailureAndKeepsIt()
+	{
+		string? failure = RepoBrowsing.Delete(Entry("src"), _repoA);
+
+		Assert.IsNotNull(failure);
+		Assert.IsTrue(File.Exists(Path.Join(_repoA, "src", "a.cs")));
+	}
+}

ProjectDirector/ProjectDirector.cs

@@ -19,7 +19,7 @@
 using ktsu.ImGui.Widgets;
 using ktsu.ImGui.Styler;
 using Octokit;
 // using OpenAI.Chat;
 using Semantics.Paths;
 
 #pragma warning disable CA1506
@@ -62,7 +62,7 @@
 	/// </summary>
 	private CloneTracker Clones { get; } = new();
 
 	// private ChatClient ChatClient { get; init; }
 
 	private static void Main(string[] _) => ImGuiApp.Start(new ProjectDirector().BuildConfig());
 
@@ -101,7 +101,7 @@
 		_ = MakeLoadedOptionsSafe(Options, QueueLog);
 
 		Options.Save();
 		// ChatClient = new(model: "gpt-4o", new ApiKeyCredential(Options.OpenAIToken));
 		DividerDiff = new("DiffDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerCols = new("VerticalDivider", DividerResized, ImGuiWidgets.DividerLayout.Columns);
 		DividerContainerRows = new("HorizontalDivider", DividerResized, ImGuiWidgets.DividerLayout.Rows);
@@ -765,7 +765,7 @@
 						});
 					}
 
 					//int fetchInterval = repo.MinFetchIntervalSeconds;
 					//if (ImGuiWidgets.Knob("Min Fetch Interval", ref fetchInterval, 0, 300, 150))
 					//{
 					//	repo.MinFetchIntervalSeconds = fetchInterval;
@@ -2074,7 +2074,9 @@
 	private void ShowCompareBrowser()
 	{
 		IEnumerable<RelativePath> allFilesystemEntries = BrowserContentsBase.Union(BrowserContentsCompare);
-		Collection<RelativePath> directories = ListBrowserDirectories(allFilesystemEntries, Options.Repos[Options.BaseRepo].LocalPath, Options.Repos[Options.CompareRepo].LocalPath);
+		string baseRoot = Options.Repos[Options.BaseRepo].LocalPath;
+		string compareRoot = Options.Repos[Options.CompareRepo].LocalPath;
+		Collection<RelativePath> directories = ListBrowserDirectories(allFilesystemEntries, baseRoot, compareRoot);
 		Collection<RelativePath> files = allFilesystemEntries.Except(directories).ToCollection();
 
 		if (ImGui.BeginTable("CompareBrowser", 3, ImGuiTableFlags.Borders))
@@ -2120,7 +2122,7 @@
 						GitRepository repoB = Options.Repos[Options.CompareRepo];
 						if (repoA is GitHubRepository githubRepoA && repoB is GitHubRepository githubRepoB)
 						{
-							SwitchCompareBrowserPath(GetFullyQualifiedRepoName(githubRepoA.OwnerName, githubRepoA.RepoName), GetFullyQualifiedRepoName(githubRepoB.OwnerName, githubRepoB.RepoName), RelativeDirectoryPath.Create<RelativeDirectoryPath>(Path.Combine(Options.BrowsePath, path)));
+							SwitchCompareBrowserPath(GetFullyQualifiedRepoName(githubRepoA.OwnerName, githubRepoA.RepoName), GetFullyQualifiedRepoName(githubRepoB.OwnerName, githubRepoB.RepoName), RelativeDirectoryPath.Create<RelativeDirectoryPath>(path.WeakString));
 
 						}
 						else
@@ -2134,11 +2136,11 @@
 				{
 					if (existsInA && ImGui.ArrowButton("##Copy", ImGuiDir.Right))
 					{
-						_ = Directory.CreateDirectory(Path.Combine(Options.Repos[Options.CompareRepo].LocalPath, Options.BrowsePath, path));
+						LogBrowserFailure("Copying", path, RepoBrowsing.Copy(path, baseRoot, compareRoot));
 					}
 					else if (existsInB && ImGui.ArrowButton("##Copy", ImGuiDir.Left))
 					{
-						_ = Directory.CreateDirectory(Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path));
+						LogBrowserFailure("Copying", path, RepoBrowsing.Copy(path, compareRoot, baseRoot));
 					}
 
 					if (ImGui.IsItemHovered())
@@ -2156,11 +2158,11 @@
 				{
 					if (existsInA && ImGui.Button("X##Remove"))
 					{
-						Directory.Delete(Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path));
+						LogBrowserFailure("Deleting", path, RepoBrowsing.Delete(path, baseRoot));
 					}
 					else if (existsInB && ImGui.Button("X##Remove"))
 					{
-						Directory.Delete(Path.Combine(Options.Repos[Options.CompareRepo].LocalPath, Options.BrowsePath, path));
+						LogBrowserFailure("Deleting", path, RepoBrowsing.Delete(path, compareRoot));
 					}
 
 					if (ImGui.IsItemHovered())
@@ -2190,15 +2192,11 @@
 				{
 					if (existsInA && ImGui.ArrowButton("##Copy", ImGuiDir.Right))
 					{
-						string srcPath = Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path);
-						string dstPath = Path.Combine(Options.Repos[Options.CompareRepo].LocalPath, Options.BrowsePath, path);
-						File.Copy(srcPath, dstPath);
+						LogBrowserFailure("Copying", path, RepoBrowsing.Copy(path, baseRoot, compareRoot));
 					}
 					else if (existsInB && ImGui.ArrowButton("##Copy", ImGuiDir.Left))
 					{
-						string srcPath = Path.Combine(Options.Repos[Options.CompareRepo].LocalPath, Options.BrowsePath, path);
-						string dstPath = Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path);
-						File.Copy(srcPath, dstPath);
+						LogBrowserFailure("Copying", path, RepoBrowsing.Copy(path, compareRoot, baseRoot));
 					}
 
 					if (ImGui.IsItemHovered())
@@ -2216,11 +2214,11 @@
 				{
 					if (existsInA && ImGui.Button("X##Remove"))
 					{
-						File.Delete(Path.Combine(Options.Repos[Options.BaseRepo].LocalPath, Options.BrowsePath, path));
+						LogBrowserFailure("Deleting", path, RepoBrowsing.Delete(path, baseRoot));
 					}
 					else if (existsInB && ImGui.Button("X##Remove"))
 					{
-						File.Delete(Path.Combine(Options.Repos[Options.CompareRepo].LocalPath, Options.BrowsePath, path));
+						LogBrowserFailure("Deleting", path, RepoBrowsing.Delete(path, compareRoot));
 					}
 
 					if (ImGui.IsItemHovered())
@@ -2300,7 +2298,7 @@
 
 				if (ImGui.TableNextColumn())
 				{
 					//if (ImGui.Button($"Propagate Directory###Propagate{path.Replace(Path.DirectorySeparatorChar, '.').Replace(Path.AltDirectorySeparatorChar, '.')}"))
 					//{
 					//	shouldOpenPopup |= true;
 					//	Options.PropagatePath = path;
@@ -2416,17 +2414,13 @@
 		_ = PopupPropagateFile.ShowIfOpen();
 	}
 
-	/// <summary>
-	/// Lists a directory for the repository browsers, in name order.
-	/// </summary>
-	/// <param name="directory">The directory to list.</param>
-	/// <returns>The full paths of its files and directories, ordered by name ignoring case.</returns>
-	/// <remarks>
-	/// The file system's own order is whatever that file system keeps: alphabetical on NTFS, by hash
-	/// on ext4. Sorting makes the browsers read the same on every machine.
-	/// </remarks>
-	internal static IEnumerable<string> ListBrowserEntries(string directory) =>
-		Directory.EnumerateFileSystemEntries(directory).Order(StringComparer.OrdinalIgnoreCase);
+	private void LogBrowserFailure(string action, RelativePath path, string? failure)
+	{
+		if (failure is not null)
+		{
+			QueueLog($"{action} {path} failed: {failure}");
+		}
+	}
 
 	/// <summary>
 	/// Picks out the entries of a browser listing that are directories in any of the given repositories.
@@ -2440,36 +2434,19 @@
 	/// compare browser used to list every directory as a file that could not be opened.
 	/// </remarks>
 	internal static Collection<RelativePath> ListBrowserDirectories(IEnumerable<RelativePath> entries, params string[] repoRoots) =>
-		entries.Where(entry => repoRoots.Any(root => Directory.Exists(Path.Join(root, entry)))).ToCollection();
+		entries.Where(entry => RepoBrowsing.IsDirectory(entry, repoRoots)).ToCollection();
 
 	private void SwitchCompareBrowserPath(FullyQualifiedGitHubRepoName baseRepo, FullyQualifiedGitHubRepoName compareRepo, RelativeDirectoryPath newPath)
 	{
 		Options.BrowsePath = newPath;
 		GitRepository repoA = Options.Repos[baseRepo];
 		GitRepository repoB = Options.Repos[compareRepo];
 
-		static RelativePath formatPath(string path, string prefix) => RelativePath.Create<RelativePath>(path.RemovePrefix(prefix + Path.DirectorySeparatorChar) + (Directory.Exists(path) ? Path.DirectorySeparatorChar : string.Empty));
-
 		BrowserContentsBase.Clear();
 		BrowserContentsCompare.Clear();
 
-		try
-		{
-			BrowserContentsBase = ListBrowserEntries(Path.Join(repoA.LocalPath, Options.BrowsePath)).Select(x => formatPath(x, repoA.LocalPath)).ToCollection();
-		}
-		catch (DirectoryNotFoundException)
-		{
-			// skip this repo
-		}
-
-		try
-		{
-			BrowserContentsCompare = ListBrowserEntries(Path.Join(repoB.LocalPath, Options.BrowsePath)).Select(x => formatPath(x, repoB.LocalPath)).ToCollection();
-		}
-		catch (DirectoryNotFoundException)
-		{
-			// skip this repo
-		}
+		BrowserContentsBase = RepoBrowsing.List(repoA.LocalPath, Options.BrowsePath);
+		BrowserContentsCompare = RepoBrowsing.List(repoB.LocalPath, Options.BrowsePath);
 
 		QueueSaveOptions();
 	}
@@ -2484,19 +2461,10 @@
 		Options.BrowsePath = newPath;
 		GitRepository repoA = Options.Repos[baseRepo];
 
-		static RelativePath formatPath(string path, string prefix) => RelativePath.Create<RelativePath>(path.RemovePrefix(prefix + Path.DirectorySeparatorChar) + (Directory.Exists(path) ? Path.DirectorySeparatorChar : string.Empty));
-
 		BrowserContentsBase.Clear();
 		BrowserContentsCompare.Clear();
 
-		try
-		{
-			BrowserContentsBase = ListBrowserEntries(Path.Join(repoA.LocalPath, Options.BrowsePath)).Select(x => formatPath(x, repoA.LocalPath)).ToCollection();
-		}
-		catch (DirectoryNotFoundException)
-		{
-			// skip this repo
-		}
+		BrowserContentsBase = RepoBrowsing.List(repoA.LocalPath, Options.BrowsePath);
 
 		QueueSaveOptions();
 	}