Skip to content
Draft
Show file tree
Hide file tree
Changes from 11 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
90 changes: 89 additions & 1 deletion src/BlazorWebView/src/Maui/BlazorWebView.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
using System;
using System.IO;
using System.Runtime.Versioning;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components.Web;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.FileProviders;
using Microsoft.Maui;
using Microsoft.Maui.Controls;
Expand Down Expand Up @@ -50,6 +52,43 @@ public BlazorWebView()
/// </summary>
public string? HostPage { get; set; }

/// <summary>
/// The synthetic host page path used when <see cref="AppType"/> renders the host document.
/// </summary>
internal const string AppTypeHostPage = "wwwroot/index.html";

private Type? _appType;
private bool _appTypeRendered;
private string? _renderedHostPageHtml;

/// <summary>
/// Gets or sets the type of a root component that renders the entire host HTML document (the
/// hybrid equivalent of a Blazor Web App's <c>App.razor</c>).
/// <para>
/// When set, the component is statically rendered to produce the host page, so a physical
/// <see cref="HostPage"/> file (such as <c>wwwroot/index.html</c>) is not required. Interactive
/// components declared inside it with a render mode (for example
/// <c>&lt;Routes @rendermode="InteractiveAuto" /&gt;</c> or
/// <c>&lt;HeadOutlet @rendermode="InteractiveAuto" /&gt;</c>) are automatically attached to the
/// live document, so an explicit <see cref="RootComponents"/> entry is not required either.
/// </para>
/// </summary>
public Type? AppType

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ Trimming/AOTAppType is a public Type? with no [DynamicallyAccessedMembers], but it is passed to HybridHostPageRenderer.Render(..., [DynamicallyAccessedMembers(All)] Type appComponentType, ...). The resulting IL2072 is silenced with an UnconditionalSuppressMessage (line 191) instead of propagating the annotation.

The suppression justification says the Razor SDK trimming roots preserve these types "consistent with RootComponent.ComponentType" — but RootComponent.ComponentType is annotated with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]; that annotation is exactly what makes the flow safe there, not an SDK root. Here the annotation chain is broken at the public entry point, so a component type assigned only via AppType (never referenced from a RootComponent, @rendermode in a rooted file, or routable assembly) can have its members trimmed and fail at render time in a trimmed/NativeAOT publish.

Annotate the property and its interface declaration instead of suppressing:

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
public Type? AppType { get; set; }

then the UnconditionalSuppressMessage at line 191 can be dropped.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — this is the right call. I'll annotate public Type? AppType (and the IBlazorWebView declaration) with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] so the trim contract propagates from the public entry point, and drop the UnconditionalSuppressMessage at line 191. You're right that the justification conflated an SDK root with the annotation on RootComponent.ComponentType — propagating the annotation is the correct fix, not silencing it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up after implementing this (commit a06e3a2): I tried annotating AppType/IBlazorWebView.AppType with [DynamicallyAccessedMembers(All)] as agreed, but it does not build clean — it produces new trim errors:

  • IL2114 on the backing field (a DAM field referenced by a reflectable type), and
  • IL2111 on the setter ("method with DAM parameters is accessed via reflection") — because AppType is set by XAML via reflection (AppType="{x:Type components:App}"), and a DAM requirement on a reflection-set property/parameter isn't satisfiable by the trimmer.

This is the same IL2111/IL2114 outcome the review's own Report section anticipated for the pr-plus-reviewer candidate. So annotation isn't viable here. I kept a narrowly-scoped IL2072 suppression but rewrote the justification to be accurate: the component type is preserved by the XAML compiler ({x:Type} roots it) and by the Razor SDK trimming roots (@rendermode/routable assembly keep the interactive components), and DAM-on-parameter is incompatible with XAML's reflection-based property setting. The old, incorrect "consistent with RootComponent.ComponentType" wording is gone. Build is clean with trimming enabled (illink runs, 0 warnings).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Handler Mapper and Property Patterns — the AppType setter never invalidates the render cache and its HostPage side effect is irreversible.

  1. Assigning a different AppType after the first CreateFileProvider leaves _appTypeRendered == true and _renderedHostPageHtml holding the previous component's document, so the old host page keeps being served while the old component's RootComponents entries (added in EnsureAppTypeRendered) remain in the collection — the new document never appears and stale roots accumulate.
  2. The setter writes HostPage = AppTypeHostPage but setting AppType = null later does not undo it, so the view is left pointing at wwwroot/index.html, a file the migrated templates no longer ship → blank page.

Reset _appTypeRendered/_renderedHostPageHtml, remove the previously-registered roots, and restore HostPage when the value changes (or document AppType as write-once and throw on reassignment after render).

{
get => _appType;
set
{
_appType = value;

// Provide a synthetic host page so the existing startup and relative-path logic flows
// unchanged; the rendered document is overlaid onto the file provider at this path.
if (value is not null && string.IsNullOrEmpty(HostPage))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

💡 Logic / correctness — asymmetric setter — The setter assigns the synthetic HostPage when AppType is set, but performs no inverse action when AppType is set back to null: HostPage is left pointing at "wwwroot/index.html", and _renderedHostPageHtml / _appTypeRendered retain their values.

If a caller clears AppType (or sets it before deciding on a different host page), the view is left claiming a host page file that may not exist on disk, and CreateFileProvider now takes the AppType is null early-return (line 172) and returns the bare platform provider — so nothing serves wwwroot/index.html and the WebView renders blank. Either restore the previous HostPage when clearing AppType, or reject the transition with a clear exception.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct edge case — clearing AppType after it was set leaves the synthetic HostPage (wwwroot/index.html) in place and keeps _renderedHostPageHtml/_appTypeRendered, while CreateFileProvider now takes the AppType is null early-return and serves the bare platform provider, so nothing renders. I'll make the setter symmetric: restore the previous HostPage and reset the rendered state when AppType is cleared (falling back to a clear exception if the transition is ambiguous). This pairs with the failure-latch fix above.

{
HostPage = AppTypeHostPage;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ Handler mapper / property patterns — The AppType setter mutates HostPage (and AppType itself) as plain CLR state, but neither is a BindableProperty and neither raises Handler?.UpdateValue(...).

Consequence: AppType is only honoured if it is assigned before the handler is connected (as all three new device tests do). Setting it on an already-realized BlazorWebView — e.g. myBlazorWebView.AppType = typeof(App); from code-behind after the page is displayed — silently does nothing: MapAppType never re-runs and the webview keeps whatever host page it started with. There is also no coverage for this ordering.

At minimum add Handler?.UpdateValue(nameof(AppType)); at the end of the setter (and consider making it a BindableProperty for XAML/binding parity with HostPage/StartPath).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair callout. Note this is consistent with the existing contract: neither HostPage nor RootComponents is a BindableProperty and neither re-runs on runtime re-assignment — they're all honoured only when set before the handler connects, which every real usage (XAML + the templates) does. So AppType matches the established set-before-connect semantics rather than introducing a new gap. Making it a BindableProperty that raises Handler?.UpdateValue(...) for post-connect re-assignment is a reasonable enhancement; I'll track it as a follow-up so this PR keeps parity with the sibling properties.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Handler Mapper and Property Patterns — Setting AppType after the handler is connected silently does nothing: the setter mutates _appType/HostPage but never calls Handler?.UpdateValue(nameof(AppType)), and EnsureAppTypeRendered is latched by _appTypeRendered anyway. The setter also has an order-dependent side effect — it only defaults HostPage when HostPage is currently empty — so <BlazorWebView HostPage="..." AppType="..."/> and <BlazorWebView AppType="..." HostPage="..."/> behave differently in XAML (attribute order determines whether the rendered document is overlaid at the user's host-page path or at the synthetic wwwroot/index.html). At minimum document that AppType must be assigned before the handler is created, and make the HostPage interaction explicit rather than silently order-sensitive.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid and a genuinely new angle — the attribute-order sensitivity is a real footgun: because the setter only defaults HostPage when it's currently empty, <BlazorWebView HostPage="..." AppType="..."/> overlays at the user's host-page path while <BlazorWebView AppType="..." HostPage="..."/> overlays at the synthetic wwwroot/index.html. I'll make this explicit rather than order-sensitive: AppType and an explicit HostPage are mutually exclusive, so I'll either reject the combination with a clear exception or define a single deterministic precedence regardless of attribute order, and document it.

}
}
}

/// <summary>
/// Bindable property for <see cref="StartPath"/>.
/// </summary>
Expand Down Expand Up @@ -124,7 +163,56 @@ public string StartPath
public virtual IFileProvider CreateFileProvider(string contentRootDir)
{
// Call into the platform-specific code to get that platform's asset file provider
return GetBlazorWebViewHandler().CreateFileProvider(contentRootDir);
var platformFileProvider = GetBlazorWebViewHandler().CreateFileProvider(contentRootDir);

// Everything below is opt-in via AppType. For the legacy HostPage (index.html) path, return
// the platform provider unchanged so existing behaviour - including the handler's own file
// provider instance - is preserved exactly.
if (AppType is null)
{
return platformFileProvider;
}

// Load the bundled static web assets manifest (if present) so that @Assets fingerprinting
// and fingerprinted-route serving work. The manifest lives outside the web root and is read
// from the app package, so it is never served to the web view. Absent (or on platforms
// without app-package access), fingerprinting simply stays off.
var manifest = StaticWebAssetsManifest.TryLoad();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

💡 Performance / lifecycleStaticWebAssetsManifest.TryLoad() is called on every CreateFileProvider invocation, i.e. once per handler start. Each call does a blocking Task.Run(...).GetAwaiter().GetResult() on the UI thread that hits the platform app-package APIs and re-parses the JSON from scratch.

For an app with several BlazorWebView instances, or a view that is reconnected across Shell tab switches / window re-creations, the same immutable, build-time-generated manifest is read and deserialized repeatedly while blocking the UI thread. Unlike _renderedHostPageHtml, this result is not cached. A static lazily-initialized cache (the manifest cannot change during the process lifetime) would remove the repeat cost entirely.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — the manifest is immutable build output, but it's re-read from the app package and re-parsed (via a blocking Task.Run(...).GetAwaiter().GetResult()) on every CreateFileProvider, i.e. once per handler start, and unlike _renderedHostPageHtml it's not cached. I'll add a static lazily-initialized cache since it can't change during the process lifetime — removes the repeat cost and the repeated UI-thread block for multi-BlazorWebView / reconnect scenarios.


// Render the host document once. This also collects any interactive components declared with
// a render mode and registers them so they attach to the live document, and resolves @Assets
// using the manifest.
EnsureAppTypeRendered(manifest?.Assets);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Architectural Layer PlacementCreateFileProvider is public virtual and its contract (name, docs, IBlazorWebViewHandler.CreateFileProvider delegation) is a pure factory, but it now performs a blocking static render of a user component, reads app-package files off disk, and mutates RootComponents. Two concrete consequences: (a) any existing subclass that overrides CreateFileProvider to return its own provider without calling base silently loses every AppType behavior — no host page, no root registrations, blank WebView; (b) correctness of repeated invocation now depends entirely on the _appTypeRendered latch inside an unrelated method. Move the render + root-component registration into an explicit step on the handler startup path and keep CreateFileProvider free of side effects.

var hostPageRelativePath = Path.GetRelativePath(contentRootDir, HostPage!);

return new BlazorWebViewFileProvider(platformFileProvider, hostPageRelativePath, _renderedHostPageHtml, manifest);
}

[System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("Trimming", "IL2072",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Trimming and AOT Compatibility — This UnconditionalSuppressMessage("Trimming", "IL2072") is applied to the whole EnsureAppTypeRendered method and hides a genuinely reachable dynamic-metadata path: the unannotated AppType (Type?, no DynamicallyAccessedMembers) flows into HybridHostPageRenderer.Render's [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] parameter, and from there into BeginRenderingComponent and ResolveComponentForRenderMode (also DAM.All). The justification — "preserved by the Razor SDK trimming roots, consistent with RootComponent.ComponentType" — does not hold as written: RootComponent.ComponentType carries the DAM annotation itself rather than relying on a suppression. The structural fix is to annotate the API instead of suppressing: put [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] on BlazorWebView.AppType and on IBlazorWebView.AppType, which propagates the requirement to callers and removes the warning at its source. As written, a component type reached only through AppType (e.g. assigned in code rather than via a {x:Type} XAML reference) can have members trimmed in a published trimmed/NativeAOT app.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — I'll annotate BlazorWebView.AppType and IBlazorWebView.AppType with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] so the requirement propagates to callers, and remove this method-wide UnconditionalSuppressMessage. You're right the justification was wrong — RootComponent.ComponentType carries the annotation itself, it isn't relying on an SDK root.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up after implementing this (commit a06e3a2): I tried annotating AppType/IBlazorWebView.AppType with [DynamicallyAccessedMembers(All)] as agreed, but it does not build clean — it produces new trim errors:

  • IL2114 on the backing field (a DAM field referenced by a reflectable type), and
  • IL2111 on the setter ("method with DAM parameters is accessed via reflection") — because AppType is set by XAML via reflection (AppType="{x:Type components:App}"), and a DAM requirement on a reflection-set property/parameter isn't satisfiable by the trimmer.

This is the same IL2111/IL2114 outcome the review's own Report section anticipated for the pr-plus-reviewer candidate. So annotation isn't viable here. I kept a narrowly-scoped IL2072 suppression but rewrote the justification to be accurate: the component type is preserved by the XAML compiler ({x:Type} roots it) and by the Razor SDK trimming roots (@rendermode/routable assembly keep the interactive components), and DAM-on-parameter is incompatible with XAML's reflection-based property setting. The old, incorrect "consistent with RootComponent.ComponentType" wording is gone. Build is clean with trimming enabled (illink runs, 0 warnings).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Trimming and AOT Compatibility — this UnconditionalSuppressMessage("Trimming", "IL2072") hides a reachable annotation gap rather than an unreachable path. AppType is a public, unannotated System.Type? property, and HybridHostPageRenderer.Render requires [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] on its appComponentType parameter — the flow from the unannotated property to the annotated parameter is exactly what IL2072 reports, and it executes on every AppType startup. The justification cites RootComponent.ComponentType, but that member solves this with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] on the property, not with a suppression. Under PublishTrimmed/NativeAOT, a component type reaching AppType through a Type value the Razor SDK roots do not fully preserve can have members trimmed and fail at render time with no build diagnostic. Structural fix: annotate BlazorWebView.AppType and IBlazorWebView.AppType with [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] and delete the suppression.

Justification = "Blazor components referenced by AppType are preserved by the Razor SDK trimming roots, consistent with RootComponent.ComponentType.")]
private void EnsureAppTypeRendered(ResourceAssetCollection? assets)
{
if (_appTypeRendered || AppType is null)
{
return;
}

_appTypeRendered = true;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ Logic / correctness (failure latching)_appTypeRendered = true is set before HybridHostPageRenderer.Render runs, and it is never reset on failure.

If Render throws (a component OnInitializedAsync failure, a missing DI service, an invalid AppType), the first CreateFileProvider call propagates the exception, but _renderedHostPageHtml stays null and the flag stays true. Any later attempt — handler reconnect after a Shell tab switch, window re-creation, or a second BlazorWebView startup — skips rendering entirely, constructs BlazorWebViewFileProvider with hostPageHtml: null, and therefore serves nothing at wwwroot/index.html. The user sees a permanently blank WebView with no error instead of the original exception.

Set the flag only after a successful render (or store the failure and rethrow it on subsequent calls) so the failure mode stays diagnosable.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed, and this one is a real latent bug — _appTypeRendered = true is set before Render() runs, so a render exception (bad OnInitializedAsync, missing DI service, invalid AppType) latches a permanently-blank state: subsequent reconnects skip rendering and construct the provider with hostPageHtml: null. I'll set the flag only after a successful render, and store+rethrow the captured failure on later calls so it stays diagnosable instead of silently blank.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness Verification_appTypeRendered = true is set before HybridHostPageRenderer.Render(...) executes, so a render failure permanently latches the "already rendered" state while _renderedHostPageHtml stays null. Concrete scenario: the user's App.razor throws during static render (or the quiescence wait faults). The exception propagates out of CreateFileProvider and aborts the first handler connect; on the next connect (Shell tab switch, page pushed again, window re-created) a new handler calls CreateFileProvider again, EnsureAppTypeRendered early-returns, and BlazorWebViewFileProvider is constructed with hostPageHtml == null. The in-memory host page is then never served and the request for wwwroot/index.html falls through to the physical provider — where the templates in this PR have deleted index.html — producing a silently blank WebView with no error. Set the flag only after a successful render (or cache the failure explicitly and rethrow), so the failure is not converted into a silent blank page.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — and you're right that the template index.html deletion sharpens this: after a latched render failure there's no physical fallback, so it's a silent blank page. I'll set _appTypeRendered only after a successful render and capture+rethrow the failure on subsequent calls, so a broken App.razor surfaces as a diagnosable error rather than a blank WebView.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[major] Logic and Correctness_appTypeRendered = true is latched before the render runs, so a failed render is permanently sticky. If HybridHostPageRenderer.Render throws (an exception in the host component's OnInitializedAsync, a NotSupportedException from an unhandled render mode, or the services null throw above), _renderedHostPageHtml stays null and the flag stays true. On the next handler connect — Shell tab switch or page re-navigation, where DisconnectHandler sets _webviewManager = null and StartWebViewCoreIfPossible() runs again — EnsureAppTypeRendered returns immediately, BlazorWebViewFileProvider is constructed with hostPageHtml == null so no in-memory host page is registered, and the request for wwwroot/index.html falls through to the platform provider where that file does not exist (the templates delete it). Result: a permanently blank WebView with no exception and no log. Set the flag only after Render returns, or record the failure and rethrow on subsequent attempts.


var services = Handler?.MauiContext?.Services
?? throw new InvalidOperationException($"Cannot render {nameof(AppType)} because no service provider is available.");

var result = HybridHostPageRenderer.Render(services, AppType, assets);
_renderedHostPageHtml = result.Html;

foreach (var registration in result.Registrations)
{
RootComponents.Add(new RootComponent

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness VerificationEnsureAppTypeRendered mutates the caller-owned public RootComponents collection as a side effect of CreateFileProvider. Concrete scenario: a user migrating to AppType keeps their existing <RootComponent Selector="#app" ComponentType="{x:Type Routes}"/> while the host document also declares <Routes @rendermode="InteractiveAuto"/>. Both entries now target #app, so two component instances are attached to the same mount element. There is no duplicate-selector detection and no diagnostic. Either de-duplicate by selector before adding, or throw/log when an AppType-derived registration collides with a user-declared RootComponent selector.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed (pairs with the HybridHostPageRenderer.cs:125 thread). The migration scenario you describe — user keeps <RootComponent Selector="#app"/> and the host doc also declares <Routes @rendermode> — would attach two components to #app with no diagnostic. I'll de-duplicate by selector before adding and throw/log on an AppType-vs-user-declared collision, plus generate unique mount ids for multiple host-document interactive roots.

{
Selector = registration.Selector,
ComponentType = registration.ComponentType,
});
}
}

/// <summary>
Expand Down
98 changes: 98 additions & 0 deletions src/BlazorWebView/src/Maui/BlazorWebViewFileProvider.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
using System;
using System.IO;
using System.Text;
using Microsoft.Extensions.FileProviders;
using Microsoft.Extensions.Primitives;

namespace Microsoft.AspNetCore.Components.WebView.Maui
{
/// <summary>
/// Wraps the platform's physical file provider to add hybrid host-page and static web asset
/// behaviour on top of it:
/// <list type="bullet">
/// <item><description>serves the in-memory rendered host page (the <see cref="BlazorWebView.AppType"/>
/// document) at the host page path, when provided; and</description></item>
/// <item><description>resolves fingerprinted request routes (for example <c>app.abc123.css</c>) to
/// their physical asset files, when a manifest is provided.</description></item>
/// </list>
/// All other requests are delegated unchanged, so existing behaviour is preserved.
/// </summary>
internal sealed class BlazorWebViewFileProvider : IFileProvider
{
private readonly IFileProvider _inner;
private readonly string? _hostPageRelativePath;
private readonly byte[]? _hostPageContents;
private readonly StaticWebAssetsManifest? _manifest;

public BlazorWebViewFileProvider(
IFileProvider inner,
string? hostPageRelativePath,
string? hostPageHtml,
StaticWebAssetsManifest? manifest)
{
_inner = inner ?? throw new ArgumentNullException(nameof(inner));
_manifest = manifest;

if (hostPageRelativePath is not null && hostPageHtml is not null)
{
_hostPageRelativePath = NormalizePath(hostPageRelativePath);
_hostPageContents = Encoding.UTF8.GetBytes(hostPageHtml);
}
}

public IFileInfo GetFileInfo(string subpath)
{
var normalized = NormalizePath(subpath);

// Serve the rendered host page from memory.
if (_hostPageContents is not null &&
string.Equals(normalized, _hostPageRelativePath, StringComparison.Ordinal))

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and Correctness — the host-page match uses StringComparison.Ordinal while the fingerprint route map a few lines below is built and queried with StringComparer.OrdinalIgnoreCase. NormalizePath also only strips leading / and converts \, so it does not collapse ./ or .. segments or normalize case. A host-page request that differs only in casing (Windows/WinUI and the iOS custom scheme handler both hand back paths whose casing is not guaranteed to match HostPage) or that arrives as ./index.html misses the in-memory document, falls through to _inner.GetFileInfo, finds nothing (the physical index.html is deleted in the migrated templates), and renders a blank page. Use one consistent comparison (and normalization) for both lookups.

{
return new InMemoryFileInfo(Path.GetFileName(_hostPageRelativePath!), _hostPageContents);
}

// If the file exists as requested, serve it directly (preserves existing behaviour).
var fileInfo = _inner.GetFileInfo(subpath);
if (fileInfo.Exists)
{
return fileInfo;
}

// Otherwise, if the request targets a fingerprinted route, serve the physical asset.
if (_manifest is not null &&
_manifest.TryResolvePhysicalPath(normalized, out var physicalPath))
{
return _inner.GetFileInfo(physicalPath);
}

return fileInfo;
}

public IDirectoryContents GetDirectoryContents(string subpath) => _inner.GetDirectoryContents(subpath);

public IChangeToken Watch(string filter) => _inner.Watch(filter);

private static string NormalizePath(string path) =>
(path ?? string.Empty).Replace('\\', '/').TrimStart('/');

private sealed class InMemoryFileInfo : IFileInfo
{
private readonly byte[] _contents;

public InMemoryFileInfo(string name, byte[] contents)
{
Name = name;
_contents = contents;
}

public bool Exists => true;
public long Length => _contents.Length;
public string? PhysicalPath => null;
public string Name { get; }
public DateTimeOffset LastModified => DateTimeOffset.UtcNow;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

💡 Logic / correctnessLastModified => DateTimeOffset.UtcNow evaluates on every access, so the same IFileInfo reports a different timestamp each time it is read.

IFileInfo.LastModified is expected to be a stable property of the file. Static-content pipelines derive Last-Modified/ETag and conditional-request handling from it; a value that changes per call makes the host page unconditionally non-cacheable and can produce inconsistent headers within a single response if the value is read more than once. Capture it once in the constructor (DateTimeOffset.UtcNow at construction, or better a deterministic value derived from the content) and return the stored field.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed — LastModified => DateTimeOffset.UtcNow recomputes on every read, so the same IFileInfo reports a moving timestamp, making the host page unconditionally non-cacheable and potentially header-inconsistent within one response. I'll capture it once in the constructor and return the stored field.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Logic and CorrectnessLastModified => DateTimeOffset.UtcNow returns a different value on every property read, violating the IFileInfo contract that the value describes the file. Two reads of the same IFileInfo instance disagree, and the static-content pipeline in this same assembly (StaticContentResponseCache / StaticContentCacheControlProvider) plus any conditional-GET / Last-Modified handling sees a perpetually-changing timestamp for the host page, so host-page caching can never validate. Capture a single DateTimeOffset in the constructor and return it.

public bool IsDirectory => false;

public Stream CreateReadStream() => new MemoryStream(_contents, writable: false);
}
}
}
22 changes: 22 additions & 0 deletions src/BlazorWebView/src/Maui/BlazorWebViewHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ public partial class BlazorWebViewHandler : IBlazorWebViewHandler
public static PropertyMapper<IBlazorWebView, BlazorWebViewHandler> BlazorWebViewMapper = new(ViewMapper)
{
[nameof(IBlazorWebView.HostPage)] = MapHostPage,
[nameof(IBlazorWebView.AppType)] = MapAppType,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Handler Mapper and Property Patterns — registering an AppType mapper entry implies the property is observable, but BlazorWebView.AppType is a plain CLR property with no BindableProperty and no Handler?.UpdateValue(nameof(AppType)) call in its setter. The entry therefore only ever executes during the initial mapper pass at handler connect; assigning AppType afterwards never reaches the handler. Combined with the fact that MapAppType is a no-op after MapHostPage, this mapper entry can never do anything. If post-connect assignment is meant to be unsupported, drop the mapper entry; if it is meant to work, add Handler?.UpdateValue(nameof(AppType)) and make MapAppType invalidate and restart.

[nameof(IBlazorWebView.RootComponents)] = MapRootComponents,
#if WINDOWS
[nameof(IView.FlowDirection)] = MapFlowDirection,
Expand Down Expand Up @@ -73,6 +74,27 @@ public static void MapHostPage(BlazorWebViewHandler handler, IBlazorWebView webV
#endif
}

/// <summary>
/// Maps the <see cref="IBlazorWebView.AppType"/> property to the specified handler.
/// </summary>
/// <param name="handler">The <see cref="BlazorWebViewHandler"/>.</param>
/// <param name="webView">The <see cref="IBlazorWebView"/>.</param>
public static void MapAppType(BlazorWebViewHandler handler, IBlazorWebView webView)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Public API Surface DesignMapAppType is dead code being added as permanent public API. The AppType setter assigns HostPage, and MapHostPage is registered before MapAppType in BlazorWebViewMapper, so by the time MapAppType runs _webviewManager is already non-null and StartWebViewCoreIfPossible() returns at its first guard; the two statements here are byte-identical to MapHostPage's body, which already ran with the same webView.HostPage value. This is being recorded in PublicAPI.Unshipped.txt as public static, so it can never be removed once shipped. Either delete the mapper entry and the method, or give it real behavior (invalidate + restart when AppType changes).

{
#if !(NETSTANDARD || !PLATFORM)
// Only views that opt into AppType need this mapper. When AppType is null the legacy
// HostPage startup path is left completely untouched (MapHostPage already handled it).
if (webView.AppType is null)
{
return;
}

// AppType provides a synthetic HostPage, so ensure the handler picks it up and attempts startup.
handler.HostPage = webView.HostPage;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

⚠️ Logic / mapper orderingMapAppType is registered between MapHostPage and MapRootComponents, and its body is identical to what MapHostPage already did (handler.HostPage = webView.HostPage; handler.StartWebViewCoreIfPossible();). Because the AppType setter assigns the synthetic HostPage eagerly, MapHostPage has already copied the same value and already attempted startup by the time this runs — so this mapper adds no behaviour on the happy path.

What it does add is an ordering hazard: it invokes StartWebViewCoreIfPossible() while handler.RootComponents is still null (that field is only assigned in MapRootComponents, which runs after this entry). If startup succeeds at this point, the platform StartWebViewCoreIfPossible (see Android/BlazorWebViewHandler.Android.cs ~L197 if (RootComponents != null)) creates _webviewManager and adds zero root components — including the AppType-derived registrations that CreateFileProvider just appended to VirtualView.RootComponents — and the subsequent MapRootComponents call returns early on _webviewManager != null, so nothing ever attaches and the page renders as a static document with a dead #app div.

Either delete this mapper (redundant, and it is now permanent public API — see PublicAPI.Unshipped.txt), or move the AppType entry after RootComponents in the mapper so the ordering invariant is explicit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordering observation is correct in the abstract, but the predicted outcome (startup with zero components → dead #app) does not manifest — the three AppType tests pass on Android (44/48, 0 failed, all 3 AppType methods green) and MacCatalyst (45/46), and the legacy tests in the same class pass too. I re-ran the full BlazorWebView device-test class on a real arm64 Android 16 emulator to confirm.

Why it works: MapRootComponents does handler.RootComponents = webView.RootComponents — a reference assignment to the same RootComponentsCollection. At the first successful StartWebViewCoreIfPossible (gated on PlatformView+Services, which is ready at/after the RootComponents map), CreateFileProvider (Android L178) runs EnsureAppTypeRendered, which appends the AppType registrations to that shared collection before the foreach (RootComponents) at L197 consumes it. So the registrations are visible and attach correctly.

That said, this correctness currently relies on that reference-sharing + connection-ordering coincidence, which is exactly the fragility you're pointing at. I'll harden it by registering the derived root components and rendering the host document before StartWebViewCoreIfPossible, so the AppType path is robust regardless of mapper ordering, and simplify/remove this now-redundant mapper (I'll keep the null early-return behavior). Thanks — this is the most useful structural note in the review.

handler.StartWebViewCoreIfPossible();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 AI-Generated Review (multi-model)

[moderate] Handler Mapper and Property PatternsMapAppType has no reachable effect, yet it is added permanently to the public API surface (static BlazorWebViewHandler.MapAppType is listed in every PublicAPI.Unshipped.txt in this PR). Trace: the AppType setter already assigns HostPage (BlazorWebView.cs:87), and MapHostPage — which runs first in BlazorWebViewMapper — sets handler.HostPage and calls StartWebViewCoreIfPossible(). By the time MapAppType runs, _webviewManager is non-null and StartWebViewCoreIfPossible() returns immediately at its _webviewManager != null guard, and handler.HostPage = webView.HostPage re-assigns the identical value. The mapper also cannot fire later, because AppType is a plain CLR property that never calls Handler.UpdateValue(nameof(AppType)). Either wire the property to invalidate through the mapper (so the entry is meaningful) or drop the mapper entry and the public MapAppType method — a public static method that cannot be removed once shipped should not be dead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the accurate framing — thanks for correcting the earlier ordering theory. Agreed MapAppType is effectively a no-op today (MapHostPage runs first and starts, the _webviewManager != null guard short-circuits this, and AppType never calls UpdateValue), yet it's now permanent public API. I'll drop the dead MapAppType entry + public method as part of moving the AppType render/registration to happen explicitly before StartWebViewCoreIfPossible (so nothing relies on the mapper firing at all).

#endif
}

/// <summary>
/// Maps the <see cref="IBlazorWebView.RootComponents"/> property to the specified handler.
/// </summary>
Expand Down
Loading
Loading