Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,16 @@

private DevbookDocument? _selected;
private DevbookChapterContent _chapter = DevbookChapterContent.None;
private int _chapterRead;
/// <summary>Resolved once when the panel starts, never per read: a read a
/// folder change starts can outlive the scope that built the panel, and a
/// provider asked then has already been disposed.</summary>
private DevbookChapterFileReader _chapterReader = DevbookChapterFileReader.Disk;
private DevbookDocument? _chapterDocument;
/// <summary>The file of the chapter read that last landed, which is what the
/// editing mode describes. Written only by the latest read, so it can lag
/// <see cref="_chapterDocument"/>, the chapter last asked for.</summary>
private string? _chapterPath;
/// <summary>The view rendering the chapter, held only so a new remark can
/// ask it what the block it was left on says. See
/// <c>FileView.BlockDigestAt</c>.</summary>
Expand Down Expand Up @@ -355,6 +364,7 @@
protected override void OnInitialized()
{
Devbook.Changed += OnDevbookChanged;
_chapterReader = Services.GetService<DevbookChapterFileReader>() ?? DevbookChapterFileReader.Disk;

_c4Store = Services.GetService<C4DevbookStore>();
if (_c4Store is not null) _c4Store.Changed += OnDevbookChanged;
Expand Down Expand Up @@ -501,6 +511,15 @@
/// different record and is read again, while a parameter set that changed
/// nothing hands back the very document already loaded and costs no read.
/// </para>
/// <para>
/// Only the latest read lands. Every parameter set starts one, so reads
/// overlap whenever the host re-renders while a file is still coming off disk,
/// and they can finish out of order: a read of the chapter that was open
/// finishing after the reader picked another one put the first chapter's text
/// under the second one's name, and an editor opened then held its buffer.
/// Everything after the read is about the chapter it read, so a dropped read
/// changes none of it.
/// </para>
/// </summary>
private async Task LoadSelectedChapterAsync()
{
Expand All @@ -513,14 +532,22 @@
// "is this a different chapter", which is what leaving the editor hangs off.
// Resetting the mode on a same-file reload would close the editor under a
// reader mid-save — the debounce re-reads through here on every write.
var sameChapter = string.Equals(_selected?.Path, _chapterDocument?.Path, StringComparison.OrdinalIgnoreCase);
// Against the chapter that last landed, not the one last asked for: a read
// this one overtakes is dropped, and it may have been the only one to see
// that the file changed — the reset below would then never run.
var sameChapter = string.Equals(_selected?.Path, _chapterPath, StringComparison.OrdinalIgnoreCase);

_chapterDocument = _selected;
var document = _selected;
_chapterDocument = document;
_chapterSaveError = null;
_chapter = _selected is null
var read = ++_chapterRead;
var chapter = _selected is null
? DevbookChapterContent.None
: await DevbookChapterContent.LoadAsync("arc42", Folders.Resolve(".arc42", RepositoryAlias), _selected.Path);
: await DevbookChapterContent.LoadAsync("arc42", Folders.Resolve(".arc42", RepositoryAlias), _selected.Path, reader: _chapterReader);
if (read != _chapterRead) return;

_chapterPath = document?.Path;
_chapter = chapter;
RefreshChapterComments();

// A different chapter is read fresh, so nothing the last one was doing
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,14 @@ internal sealed record DevbookChapterContent(DevbookChapterRef? Chapter, string?
internal static DevbookChapterContent None { get; } = new(null, null);

/// <summary>Loads against a folder the devbook-folder port located.</summary>
/// <param name="reader">The disk when omitted; see <see cref="DevbookChapterFileReader"/>.</param>
internal static Task<DevbookChapterContent> LoadAsync(
string areaKey,
DevbookFolderLocation? location,
string? selection,
CancellationToken cancellationToken = default) =>
LoadAsync(DevbookChapterResolver.TryResolve(areaKey, location, selection), cancellationToken);
CancellationToken cancellationToken = default,
DevbookChapterFileReader? reader = null) =>
LoadAsync(DevbookChapterResolver.TryResolve(areaKey, location, selection), reader, cancellationToken);

/// <summary>Loads against a root a store already holds.
/// <para>
Expand All @@ -44,21 +46,23 @@ internal static Task<DevbookChapterContent> LoadAsync(
/// the store had said read-only, the status control was hidden on that word,
/// and the edit control beside it was offered anyway.
/// </para></summary>
/// <param name="reader">The disk when omitted; see <see cref="DevbookChapterFileReader"/>.</param>
internal static Task<DevbookChapterContent> LoadAsync(
string areaKey,
string? rootPath,
string? selection,
bool canEdit = true,
CancellationToken cancellationToken = default) =>
LoadAsync(DevbookChapterResolver.TryResolve(areaKey, rootPath, selection, canEdit: canEdit), cancellationToken);
CancellationToken cancellationToken = default,
DevbookChapterFileReader? reader = null) =>
LoadAsync(DevbookChapterResolver.TryResolve(areaKey, rootPath, selection, canEdit: canEdit), reader, cancellationToken);

private static async Task<DevbookChapterContent> LoadAsync(DevbookChapterRef? chapter, CancellationToken cancellationToken)
private static async Task<DevbookChapterContent> LoadAsync(DevbookChapterRef? chapter, DevbookChapterFileReader? reader, CancellationToken cancellationToken)
{
if (chapter is null) return None;

try
{
return new DevbookChapterContent(chapter, await File.ReadAllTextAsync(chapter.FullPath, cancellationToken).ConfigureAwait(false));
return new DevbookChapterContent(chapter, await (reader ?? DevbookChapterFileReader.Disk).ReadAllTextAsync(chapter.FullPath, cancellationToken).ConfigureAwait(false));
}
catch (Exception ex) when (ex is IOException or UnauthorizedAccessException)
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
namespace Backlog.Desktop.UI.Devbook;

/// <summary>
/// How a panel's chapter comes off disk: <see cref="File.ReadAllTextAsync(string, CancellationToken)"/>,
/// and nothing else in any host.
/// <para>
/// A seam, and only a seam. A panel starts a chapter read on every parameter
/// set, so reads overlap whenever the selection moves while a file is still
/// being read, and they can finish out of order. On a fast disk they almost never
/// do, which is exactly why a test needs to hold one: registering a subclass is
/// how it makes the older read finish last on purpose. No host registers one, so
/// a panel falls back to <see cref="Disk"/>.
/// </para>
/// </summary>
internal class DevbookChapterFileReader
{
internal static DevbookChapterFileReader Disk { get; } = new();

internal virtual Task<string> ReadAllTextAsync(string path, CancellationToken cancellationToken) =>
File.ReadAllTextAsync(path, cancellationToken);
}
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,10 @@
/// so which of them the read view is already showing a control for.</summary>
private HashSet<string> _statusHeadings = new(StringComparer.OrdinalIgnoreCase);

/// <summary>The file and root of the chapter read that last landed, which is
/// what the baselines and modes describe. Written only by the latest read, so
/// they can lag <see cref="_chapterDocument"/>, the chapter last asked for.</summary>
private string? _chapterPath;
private string? _chapterRoot;
private string? _openedText;
private string? _committedText;
Expand All @@ -228,6 +232,11 @@
/// launchers rather than offering changes the next fetch would discard.</summary>
private bool CanEdit => _view?.CanEdit ?? true;
private DevbookChapterContent _chapter = DevbookChapterContent.None;
private int _chapterRead;
/// <summary>Resolved once when the panel starts, never per read: a read a
/// folder change starts can outlive the scope that built the panel, and a
/// provider asked then has already been disposed.</summary>
private DevbookChapterFileReader _chapterReader = DevbookChapterFileReader.Disk;
private DomainDevbookDocument? _chapterDocument;
/// <summary>The view rendering the chapter, held only so a new remark can
/// ask it what the block it was left on says. See
Expand Down Expand Up @@ -350,6 +359,7 @@
protected override void OnInitialized()
{
DomainDevbook.Changed += OnDevbookChanged;
_chapterReader = Services.GetService<DevbookChapterFileReader>() ?? DevbookChapterFileReader.Disk;

_c4Store = Services.GetService<C4DevbookStore>();
if (_c4Store is not null) _c4Store.Changed += OnDevbookChanged;
Expand Down Expand Up @@ -494,6 +504,15 @@
/// parent can come back with the same records after the file underneath them
/// changed.
/// </para>
/// <para>
/// Only the latest read lands. Every parameter set starts one, so reads
/// overlap whenever the host re-renders while a file is still coming off disk,
/// and they can finish out of order: a read of the chapter that was open
/// finishing after the reader picked another one put the first chapter's text
/// under the second one's name, and an editor opened then held its buffer.
/// Everything after the read is about the chapter it read, so a dropped read
/// changes none of it.
/// </para>
/// </summary>
private async Task LoadSelectedChapterAsync(bool force = false)
{
Expand All @@ -508,17 +527,24 @@
// The root belongs in the comparison too: the same relative path under a
// folder that has moved is a different file, and a baseline read from the
// one before it would compare a chapter against somebody else's copy of it.
var sameChapter = string.Equals(document?.Path, _chapterDocument?.Path, StringComparison.OrdinalIgnoreCase)
&& string.Equals(_view?.RootPath, _chapterRoot, StringComparison.OrdinalIgnoreCase);

_chapterRoot = _view?.RootPath;
// Against the chapter that last landed, not the one last asked for: a read
// this one overtakes is dropped, and it may have been the only one to see
// that the file changed — the reset below would then never run.
var root = _view?.RootPath;
var sameChapter = string.Equals(document?.Path, _chapterPath, StringComparison.OrdinalIgnoreCase)
&& string.Equals(root, _chapterRoot, StringComparison.OrdinalIgnoreCase);

_chapterDocument = document;
_chapterSaveError = null;
_chapter = _view is null || document is null
var read = ++_chapterRead;
var chapter = _view is null || document is null
? DevbookChapterContent.None
: await DevbookChapterContent.LoadAsync("domain", _view.RootPath, document.Path, _view.CanEdit);
: await DevbookChapterContent.LoadAsync("domain", _view.RootPath, document.Path, _view.CanEdit, reader: _chapterReader);
if (read != _chapterRead) return;

_chapterPath = document?.Path;
_chapterRoot = root;
_chapter = chapter;
RefreshChapterComments();

_statusHeadings = HeadingsWithStatus(_chapter.Text);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
@inject GitHubSettingsStore Repositories
@inject InstructionSourceDiscovery InstructionSources
@inject IDevbookFolderSource Folders
@inject IServiceProvider Services

@*
The eyebrow, the title and the subtitle are all gone. The tab strip says
Expand Down Expand Up @@ -164,6 +165,11 @@
private IReadOnlyList<InstructionRepositoryView> _repositories = [];
private string? _loadedRepositoryAlias;
private DevbookChapterContent _chapter = DevbookChapterContent.None;
private int _chapterRead;
/// <summary>Resolved once when the panel starts, never per read: a read a
/// folder change starts can outlive the scope that built the panel, and a
/// provider asked then has already been disposed.</summary>
private DevbookChapterFileReader _chapterReader = DevbookChapterFileReader.Disk;
private InstructionDocument? _chapterDocument;
private DevbookChapterEditor? _editor;
private bool _editing;
Expand Down Expand Up @@ -230,7 +236,11 @@

private InstructionDocument? SelectedDocument => ResolveSelectedDocument();

protected override void OnInitialized() => Folders.Changed += OnDevbookChanged;
protected override void OnInitialized()
{
Folders.Changed += OnDevbookChanged;
_chapterReader = Services.GetService<DevbookChapterFileReader>() ?? DevbookChapterFileReader.Disk;
}

/// <summary>A folder that moved or changed underneath the panel. A refresh
/// rather than a load, the same as the other four areas: the scope did not
Expand Down Expand Up @@ -402,16 +412,27 @@
/// identity so a parameter set that changed nothing costs no read, and every
/// re-discovery hands back new records, so a save is read again.
/// </para>
/// <para>
/// Only the latest read lands. Every parameter set starts one, so reads
/// overlap whenever the host re-renders while a file is still coming off disk,
/// and they can finish out of order: a read of the file that was open
/// finishing after the reader picked another one put the first file's text
/// under the second one's name, and an editor opened then held its buffer.
/// </para>
/// </summary>
private async Task LoadSelectedChapterAsync()
{
var document = SelectedDocument;
if (ReferenceEquals(_chapterDocument, document)) return;

_chapterDocument = document;
_chapter = ActiveRepository is { } repository && document is not null
? await DevbookChapterContent.LoadAsync("instructions", repository.LocalRoot, document.RelativePath, repository.CanEdit)
var read = ++_chapterRead;
var chapter = ActiveRepository is { } repository && document is not null
? await DevbookChapterContent.LoadAsync("instructions", repository.LocalRoot, document.RelativePath, repository.CanEdit, reader: _chapterReader)
: DevbookChapterContent.None;
if (read != _chapterRead) return;

_chapter = chapter;
}

/// <summary>
Expand Down
86 changes: 86 additions & 0 deletions tests/Backlog.Desktop.UI.UnitTests/Arc42DevbookPanelTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,92 @@ public async Task A_folder_that_cannot_be_read_after_a_change_leaves_the_last_ch
Assert.Contains("Original prose.", component.Markup, StringComparison.Ordinal);
}

/// <summary>
/// A folder change re-reads the open chapter, and a reader can pick another
/// one while that read is still coming off disk. The two reads then finish in
/// either order, and the older one used to land last and win: the decision's
/// text was drawn under the introduction's name, and Edit opened the decision's
/// buffer on the introduction's file.
/// </summary>
[Fact]
public async Task A_chapter_read_that_finishes_after_a_newer_one_is_dropped()
{
await using var harness = CreateHarness(withArc42Folder: true);
var reader = new HeldChapterFileReader("0001-decision.md") { Holding = false };
harness.Context.Services.AddSingleton<DevbookChapterFileReader>(reader);

var component = harness.Render(DecisionPath);
component.WaitForAssertion(() => Assert.Contains("Original prose.", component.Markup, StringComparison.Ordinal));

reader.Holding = true;
harness.Folders.NotifyContentChanged();
await reader.Started.WaitAsync(TestContext.Current.CancellationToken);

component.Render(parameters => parameters
.Add(panel => panel.RepositoryAlias, harness.RepositoryAlias)
.Add(panel => panel.SelectedPath, ".arc42/01-introduction.md"));
component.WaitForAssertion(() => Assert.Contains("Goals.", component.Markup, StringComparison.Ordinal));

// The refresh that holds the older read ends in a render whichever way
// the read is taken up, so that render is what says the stale text had
// its chance to land.
var renders = component.RenderCount;
reader.Release();
await reader.Returned.WaitAsync(TestContext.Current.CancellationToken);
component.WaitForState(() => component.RenderCount > renders);

Assert.Equal("Introduction", component.Find(".file-view__name").TextContent.Trim());
Assert.Contains("Goals.", component.Markup, StringComparison.Ordinal);
Assert.DoesNotContain("Original prose.", component.Markup, StringComparison.Ordinal);

await component.InvokeAsync(() => component.Find("[data-testid='arc42-chapter-file-edit']").Click());
component.WaitForAssertion(() => Assert.Equal(
"# Introduction\n\nGoals.\n",
component.Find("[data-testid='devbook-chapter-surface'] textarea").GetAttribute("value")));
}

/// <summary>
/// A dropped read can be the only one that saw the chapter change. The reader
/// edits the decision, picks the introduction while its read is held, and a
/// folder change re-reads the introduction before that read is back. The
/// re-read is the one that lands, and it has to know it is opening a different
/// file: decided against the chapter last asked for, it saw the introduction
/// both times and left the editor open on a file nobody chose to edit.
/// </summary>
[Fact]
public async Task A_reload_that_overtakes_a_dropped_read_still_leaves_the_editor()
{
await using var harness = CreateHarness(withArc42Folder: true);
var reader = new HeldChapterFileReader("01-introduction.md");
harness.Context.Services.AddSingleton<DevbookChapterFileReader>(reader);

var component = harness.Render(DecisionPath);
component.WaitForAssertion(() => Assert.Single(component.FindAll("[data-testid='arc42-chapter-file-edit']")));
await component.InvokeAsync(() => component.Find("[data-testid='arc42-chapter-file-edit']").Click());
component.WaitForAssertion(() => Assert.Single(component.FindAll("[data-testid='devbook-chapter-surface'] textarea")));

component.Render(parameters => parameters
.Add(panel => panel.RepositoryAlias, harness.RepositoryAlias)
.Add(panel => panel.SelectedPath, ".arc42/01-introduction.md"));
await reader.Started.WaitAsync(TestContext.Current.CancellationToken);

// The folder change's read of the same chapter goes straight through, so
// it lands first, with a record of its own past the identity guard.
reader.Holding = false;
harness.Folders.NotifyContentChanged();
component.WaitForAssertion(() => Assert.Contains("Goals.", component.Markup, StringComparison.Ordinal));

var renders = component.RenderCount;
reader.Release();
await reader.Returned.WaitAsync(TestContext.Current.CancellationToken);
component.WaitForState(() => component.RenderCount > renders);

Assert.Equal("Introduction", component.Find(".file-view__name").TextContent.Trim());
Assert.DoesNotContain("Original prose.", component.Markup, StringComparison.Ordinal);
Assert.Empty(component.FindAll("[data-testid='devbook-chapter-surface']"));
Assert.Single(component.FindAll("[data-testid='arc42-chapter-file-edit']"));
}

[Fact]
public async Task A_selected_chapter_opens_as_the_file_read_and_offers_a_way_in()
{
Expand Down
Loading
Loading