From acf024252deaa26cfde190afc13e45411abe4741 Mon Sep 17 00:00:00 2001 From: Amir Pournasserian Date: Mon, 19 Oct 2026 20:16:51 -0400 Subject: [PATCH 1/2] Carry the set view's prerendered diff into the circuit and close its test gaps Closes #2627. --- src/FluentCMS.Admin/AdminLimits.cs | 11 +++ .../Components/Pages/ChangeSetView.razor | 17 ++++- .../Components/Review/ChangeSetItems.razor | 71 ++++++++++++++++++- src/FluentCMS.Core/ChangeSets/ItemDiff.cs | 9 +++ src/FluentCMS.Core/ChangeSets/ValueDiff.cs | 7 ++ src/FluentCMS.Host/FluentCmsHost.cs | 5 +- .../Browser/BrowserSession.cs | 5 +- .../Browser/SetViewBrowserTests.cs | 62 ++++++++++++++++ .../Desk/ChangeSetScreenTests.cs | 62 ++++++++++++++-- .../FluentCMS.Tests/Admin/AdminTestContext.cs | 5 ++ .../Admin/ChangeSetViewTests.cs | 25 +++++++ .../ChangeSets/ItemDiffTests.cs | 16 +++++ 12 files changed, 281 insertions(+), 14 deletions(-) create mode 100644 src/FluentCMS.Admin/AdminLimits.cs create mode 100644 tests/FluentCMS.IntegrationTests/Browser/SetViewBrowserTests.cs diff --git a/src/FluentCMS.Admin/AdminLimits.cs b/src/FluentCMS.Admin/AdminLimits.cs new file mode 100644 index 000000000..130cdfc7c --- /dev/null +++ b/src/FluentCMS.Admin/AdminLimits.cs @@ -0,0 +1,11 @@ +namespace FluentCMS.Admin; + +// Sizes the host and the admin's components must agree on. +public static class AdminLimits +{ + // The largest message the admin's circuit hub takes, so a component's prerendered state, carried in the circuit's first message, + // fits. A component keeps its state only while it stays well under this. + public const long CircuitStartBytes = 1024 * 1024; + + public const int KeptStateBytes = 768 * 1024; +} diff --git a/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor b/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor index 90bfed11b..638bfcb0e 100644 --- a/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor +++ b/src/FluentCMS.Admin/Components/Pages/ChangeSetView.razor @@ -185,6 +185,10 @@ @if (Openable(source) is { } address) { @source + @if (AsciiHost(address) is { } host) + { + @L["(host {0})", host] + } } else { @@ -607,6 +611,13 @@ _ => L["The set was reverted. Nothing more can be done with it."], }; + // A host written with other than ASCII letters, such as a Cyrillic "а" in a look-alike of apple.com, is shown as the host it + // resolves to, so a reader sees xn--pple-43d.com rather than what looks like apple.com. + public static string? AsciiHost(string address) + { + return Uri.TryCreate(address, UriKind.Absolute, out var uri) && uri.Host != uri.IdnHost ? uri.IdnHost : null; + } + // Only a web address becomes a link, so a source such as "javascript:" or a file path is never followed. public static string? Openable(string source) { @@ -654,9 +665,9 @@ "change_set.published" => L["Published"], "change_set.rejected" => L["Rejected"], "change_set.reverted" => L["Reverted"], - _ => operation, + _ => L["Other event ({0})", operation], }; - private static RenderFragment Time(DateTimeOffset time) => - @; + private RenderFragment Time(DateTimeOffset time) => + @; } diff --git a/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor b/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor index a934aaf53..1fd2b688c 100644 --- a/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor +++ b/src/FluentCMS.Admin/Components/Review/ChangeSetItems.razor @@ -1,10 +1,14 @@ +@using System.Text.Json @using FluentCMS.Core.ChangeSets @using Microsoft.Extensions.DependencyInjection +@implements IDisposable @inject IServiceScopeFactory Scopes +@inject PersistentComponentState State @inject IStringLocalizer L @* The set view's interactive island: DiffView adds items in batches over the circuit (PERF-04) while the rest of the page stays static. *@ -@* The circuit replaces the prerendered items when it attaches; data-interactive says it has, for tests that drive the keyboard. *@ +@* The prerendered diff is carried into the circuit, so the circuit shows the same items without reading them again or emptying the + list. data-interactive says the circuit has attached, for tests that drive the keyboard. *@
@if (_error is not null) { @@ -23,9 +27,12 @@ @code { private ChangeSetDiff? _diff; private string? _error; + private PersistingComponentStateSubscription? _persisting; [Parameter, EditorRequired] public Guid ChangeSetId { get; set; } + private string Key => $"change-set-items-{ChangeSetId:N}"; + protected override async Task OnParametersSetAsync() { if (_diff?.ChangeSetId == ChangeSetId) @@ -33,6 +40,12 @@ return; } + if (State.TryTakeFromJson(Key, out var kept) && kept is not null) + { + _diff = kept.ToDiff(); + return; + } + // Prerendered while the set view may still be loading: an async read on PostgreSQL can finish at once, so the view renders the // island before its own reads are done. Its own scope keeps the two off one DbContext. await using var scope = Scopes.CreateAsyncScope(); @@ -40,6 +53,7 @@ if (diff.IsSuccess) { (_diff, _error) = (diff.Value, null); + _persisting ??= State.RegisterOnPersisting(Keep, RenderMode.InteractiveServer); } else { @@ -51,4 +65,59 @@ })); } } + + // A diff too large for the circuit's first message is left for the circuit to read again. + private Task Keep() + { + if (_diff is { } diff) + { + var kept = KeptDiff.From(diff); + if (JsonSerializer.SerializeToUtf8Bytes(kept).Length <= AdminLimits.KeptStateBytes) + { + State.PersistAsJson(Key, kept); + } + } + + return Task.CompletedTask; + } + + public void Dispose() => _persisting?.Dispose(); + + private sealed record KeptDiff(Guid ChangeSetId, KeptItem[] Items) + { + public static KeptDiff From(ChangeSetDiff diff) => new(diff.ChangeSetId, [.. diff.Items.Select(KeptItem.From)]); + + public ChangeSetDiff ToDiff() => new(ChangeSetId, [.. Items.Select(item => item.ToDiff())]); + } + + private sealed record KeptItem( + OperationKind Kind, + ItemKind ItemKind, + Guid ItemId, + int? BaseVersion, + int? ProposedVersion, + KeptValue[] Properties, + KeptValue[] Fields) + { + public static KeptItem From(ItemDiff item) => new( + item.Operation.Kind, + item.Operation.ItemKind, + item.Operation.ItemId, + item.Operation.BaseVersion, + item.Operation.ProposedVersion, + [.. item.Properties.Select(KeptValue.From)], + [.. item.Fields.Select(KeptValue.From)]); + + public ItemDiff ToDiff() => ItemDiff.Restore( + ChangeOperation.Restore(Kind, ItemKind, ItemId, BaseVersion, ProposedVersion), + [.. Properties.Select(value => value.ToDiff())], + [.. Fields.Select(value => value.ToDiff())]); + } + + private sealed record KeptValue(string Name, ValueChange Change, JsonElement? Before, JsonElement? After) + { + public static KeptValue From(ValueDiff value) => new(value.Name, value.Change, value.Before, value.After); + + public ValueDiff ToDiff() => ValueDiff.Restore(Name, Change, Before, After); + } } diff --git a/src/FluentCMS.Core/ChangeSets/ItemDiff.cs b/src/FluentCMS.Core/ChangeSets/ItemDiff.cs index 1671b30f2..28ee4b6fd 100644 --- a/src/FluentCMS.Core/ChangeSets/ItemDiff.cs +++ b/src/FluentCMS.Core/ChangeSets/ItemDiff.cs @@ -28,6 +28,15 @@ private ItemDiff(ChangeOperation operation, IReadOnlyList properties, public IReadOnlyList Fields { get; } + // A diff read back from where it was kept, such as the state the set view carries into its circuit. + public static ItemDiff Restore(ChangeOperation operation, IReadOnlyList properties, IReadOnlyList fields) + { + ArgumentNullException.ThrowIfNull(operation); + ArgumentNullException.ThrowIfNull(properties); + ArgumentNullException.ThrowIfNull(fields); + return new ItemDiff(operation, properties, fields); + } + // The base definition is absent for a create and the proposed one for a delete, as the operation's versions say. A field's // value is its definition without the name. public static ItemDiff ForSchema(ChangeOperation operation, ContentTypeDefinition? baseDefinition, ContentTypeDefinition? proposedDefinition) diff --git a/src/FluentCMS.Core/ChangeSets/ValueDiff.cs b/src/FluentCMS.Core/ChangeSets/ValueDiff.cs index a8fac5b90..bad9b02d5 100644 --- a/src/FluentCMS.Core/ChangeSets/ValueDiff.cs +++ b/src/FluentCMS.Core/ChangeSets/ValueDiff.cs @@ -23,6 +23,13 @@ private ValueDiff(string name, ValueChange change, JsonElement? before, JsonElem public JsonElement? After { get; } + // A diff read back from where it was kept, such as the state the set view carries into its circuit. + public static ValueDiff Restore(string name, ValueChange change, JsonElement? before, JsonElement? after) + { + ArgumentException.ThrowIfNullOrEmpty(name); + return new ValueDiff(name, change, before, after); + } + // Values are matched by exact name and compared as JSON, so property order and number formatting are not changes. The result // follows the order of the later version, then lists the removed values in the order of the earlier one. public static IReadOnlyList Compare( diff --git a/src/FluentCMS.Host/FluentCmsHost.cs b/src/FluentCMS.Host/FluentCmsHost.cs index a6cf30a8f..01975ba54 100644 --- a/src/FluentCMS.Host/FluentCmsHost.cs +++ b/src/FluentCMS.Host/FluentCmsHost.cs @@ -56,7 +56,10 @@ public static WebApplication BuildWebApplication(string[] args, Action hub.MaximumReceiveMessageSize = AdminLimits.CircuitStartBytes); RunJobsAsTheirInitiator(builder.Services); var app = builder.Build(); diff --git a/tests/FluentCMS.IntegrationTests/Browser/BrowserSession.cs b/tests/FluentCMS.IntegrationTests/Browser/BrowserSession.cs index 4857bf7c5..a24cd2ba7 100644 --- a/tests/FluentCMS.IntegrationTests/Browser/BrowserSession.cs +++ b/tests/FluentCMS.IntegrationTests/Browser/BrowserSession.cs @@ -27,10 +27,11 @@ private BrowserSession(TemporaryInstall install, AdminHost host, Guid ownerId, I public string BaseUrl => Host.Client.BaseAddress!.ToString().TrimEnd('/'); - public static async Task StartAsync() + // Settings such as a TemporaryDatabase's configuration run the host on another provider. + public static async Task StartAsync(IDictionary? settings = null) { var install = new TemporaryInstall(); - var host = await AdminHost.StartAsync(install); + var host = await AdminHost.StartAsync(install, settings: settings); var ownerId = await host.CreateFirstAdministratorAsync(); var playwright = await Playwright.CreateAsync(); var browser = await playwright.Chromium.LaunchAsync(new BrowserTypeLaunchOptions { Headless = true }); diff --git a/tests/FluentCMS.IntegrationTests/Browser/SetViewBrowserTests.cs b/tests/FluentCMS.IntegrationTests/Browser/SetViewBrowserTests.cs new file mode 100644 index 000000000..f82791fd6 --- /dev/null +++ b/tests/FluentCMS.IntegrationTests/Browser/SetViewBrowserTests.cs @@ -0,0 +1,62 @@ +using FluentCMS.Core.ChangeSets; +using FluentCMS.Core.Schema; +using FluentCMS.Infrastructure.Persistence; +using Microsoft.Playwright; + +namespace FluentCMS.IntegrationTests.Browser; + +public sealed class SetViewBrowserTests +{ + // PERF-04: a 200-item set arrives in batches over the circuit. The circuit starts from the items the page arrived with, so the list + // never empties when it attaches; on PostgreSQL a second read used to show "Loading the items…" in their place. + [BrowserFact] + public async Task A_large_set_arrives_in_batches_and_the_list_never_empties_when_the_circuit_attaches() + { + var provider = string.IsNullOrWhiteSpace(Environment.GetEnvironmentVariable(PostgresFactAttribute.VariableName)) + ? DatabaseProviders.Sqlite + : DatabaseProviders.PostgreSql; + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var session = await BrowserSession.StartAsync(database.Configuration); + var set = await AwaitingReviewAsync(session, 200); + var page = session.Page; + await session.SignInWithKeyboardAsync(); + + // The fewest items shown at any moment after the page arrived with its prerendered items. + await page.AddInitScriptAsync(""" + document.addEventListener("DOMContentLoaded", () => { + const shown = () => document.querySelectorAll('[data-slot="diff-view-item"]').length; + window.fewestItems = shown(); + new MutationObserver(() => { window.fewestItems = Math.min(window.fewestItems, shown()); }) + .observe(document.body, { childList: true, subtree: true }); + }); + """); + await page.GotoAsync($"{session.BaseUrl}/sets/{set.Id}"); + + await Assertions.Expect(page.Locator("[data-testid=change-set-items][data-interactive=true]")).ToBeVisibleAsync(); + await Assertions.Expect(page.Locator("[data-slot=diff-view-item]")).ToHaveCountAsync(200); + Assert.True(await page.EvaluateAsync("window.fewestItems") >= 25, "The list emptied when the circuit attached."); + } + + private static async Task AwaitingReviewAsync(BrowserSession session, int items) + { + var types = new List(); + for (var index = 0; index < items; index++) + { + var slug = $"type-{index}"; + types.Add(await session.Host.AsUserAsync(session.OwnerId, async directory => + (await directory.CreateAsync(new NewContentType(slug, slug, new ContentTypeDefinition([new FieldDefinition("title", "text")])), TestChangeSetTargets.ContentModel)).Value)); + } + + return await session.Host.AsUserAsync(session.OwnerId, async directory => + { + var set = (await directory.OpenAsync(new NewChangeSet("Add two hundred types.", []))).Value; + foreach (var type in types) + { + Assert.True((await directory.AddOperationAsync(set.Id, ChangeOperation.Create(OperationKind.Create, ItemKind.Schema, type.Id, null, 1).Value)).IsSuccess); + } + + await directory.TransitionAsync(set.Id, ChangeSetState.ChecksRunning); + return (await directory.TransitionAsync(set.Id, ChangeSetState.AwaitingReview)).Value; + }); + } +} diff --git a/tests/FluentCMS.IntegrationTests/Desk/ChangeSetScreenTests.cs b/tests/FluentCMS.IntegrationTests/Desk/ChangeSetScreenTests.cs index 5c6395bec..c63c7a89c 100644 --- a/tests/FluentCMS.IntegrationTests/Desk/ChangeSetScreenTests.cs +++ b/tests/FluentCMS.IntegrationTests/Desk/ChangeSetScreenTests.cs @@ -2,6 +2,7 @@ using System.Text.RegularExpressions; using FluentCMS.Core.ChangeSets; using FluentCMS.Core.Schema; +using FluentCMS.Infrastructure.Persistence; namespace FluentCMS.IntegrationTests.Desk; @@ -35,11 +36,14 @@ public async Task An_unknown_set_is_not_found() Assert.Contains("There is nothing at this address.", await response.Content.ReadAsStringAsync()); } - [Fact] - public async Task A_set_shows_its_rationale_sources_history_and_first_items_and_the_desk_links_to_it() + [Theory] + [InlineData(DatabaseProviders.Sqlite)] + [PostgresData] + public async Task A_set_shows_its_rationale_sources_history_and_first_items_and_the_desk_links_to_it(string provider) { using var install = new TemporaryInstall(); - await using var host = await AdminHost.StartAsync(install); + await using var database = await TemporaryDatabase.CreateAsync(provider); + await using var host = await AdminHost.StartAsync(install, settings: database.Configuration); var ownerId = await host.CreateFirstAdministratorAsync(); var types = new List(); for (var index = 0; index < 30; index++) @@ -51,7 +55,7 @@ public async Task A_set_shows_its_rationale_sources_history_and_first_items_and_ { var opened = (await directory.OpenAsync(new NewChangeSet( "Add thirty types.", - ["https://example.com/spec", "javascript:alert(1)", "specs/types.md"]))).Value; + ["https://example.com/spec", "javascript:alert(1)", "specs/types.md", "https://\u0430pple.com/prices"]))).Value; foreach (var type in types) { Assert.True((await directory.AddOperationAsync(opened.Id, Create(type))).IsSuccess); @@ -70,6 +74,8 @@ public async Task A_set_shows_its_rationale_sources_history_and_first_items_and_ Assert.Contains("href=\"https://example.com/spec\" target=\"_blank\" rel=\"noopener noreferrer\"", page); Assert.DoesNotContain("href=\"javascript:", page); Assert.Contains("specs/types.md", page); + Assert.Contains("xn--pple-43d.com", page[page.IndexOf("data-testid=\"source-host\"", StringComparison.Ordinal)..]); + Assert.Single(Regex.Matches(page, "data-testid=\"source-host\"")); Assert.Equal( ["change_set.opened", "change_set.submitted", "change_set.awaiting_review"], Regex.Matches(page[page.IndexOf("data-testid=\"change-set-history\"", StringComparison.Ordinal)..], "data-operation=\"([a-z_.]+)\"").Select(match => match.Groups[1].Value)); @@ -80,11 +86,14 @@ public async Task A_set_shows_its_rationale_sources_history_and_first_items_and_ Assert.Contains("