Skip to content

fix(Navigation): Use NullLogger fallback with diagnostic warning and add documentation to prevent crash when Region.Attached is used in ExtendedSplashScreen - #2958

Merged
kazo0 merged 9 commits into
mainfrom
copilot/fix-logger-exception-splash-screen
Nov 27, 2025
Merged

fix(Navigation): Use NullLogger fallback with diagnostic warning and add documentation to prevent crash when Region.Attached is used in ExtendedSplashScreen#2958
kazo0 merged 9 commits into
mainfrom
copilot/fix-logger-exception-splash-screen

Conversation

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

GitHub Issue (If applicable): closes #2957

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Documentation content changes

What is the current behavior?

Using Region.Attached="True" inside an ExtendedSplashScreen content throws NullReferenceException: "Logger needs to be set" because XAML parsing triggers AttachedChanged before NavigationHostedService.StartAsync initializes the logger.

<utu:ExtendedSplashScreen x:Name="Splash">
    <Grid uen:Region.Attached="True"
          uen:Region.Navigator="Visibility">
        <!--  Crashes here  -->
    </Grid>
</utu:ExtendedSplashScreen>

What is the new behavior?

The navigation system already properly defers region initialization until services are available. When Region.Attached="True" is encountered:

  1. A NavigationRegion is created and hooks up ViewLoading/ViewLoaded events
  2. When the view loads, AssignParent is called which looks for the service provider
  3. If services aren't available yet, it returns early and waits
  4. Once services become available, the region properly initializes

The only missing piece was that Region.Logger was accessed before NavigationHostedService.StartAsync ran, causing the crash. This fix:

  • Uses NullLogger<NavigationRegion>.Instance as a fallback when the logger hasn't been set yet
  • Outputs a diagnostic message via Debug.WriteLine explaining the deferred initialization behavior
  • Uses thread-safe Interlocked.CompareExchange to ensure the message only appears once

Code Changes:

// Before
internal static ILogger Logger { get => _logger ?? throw new NullReferenceException("Logger needs to be set"); ... }

// After  
internal static ILogger Logger
{
    get
    {
        if (_logger is null && Interlocked.CompareExchange(ref _loggerWarningIssued, 1, 0) == 0)
        {
            // This warning is expected when Region.Attached="True" is used before the navigation
            // host is fully started (e.g., inside ExtendedSplashScreen content). The region will
            // be properly initialized once the view is loaded and services become available.
            System.Diagnostics.Debug.WriteLine(
                "[Uno.Extensions.Navigation] Region.Attached is being used before the navigation host has fully started. " +
                "This typically happens when Region.Attached=\"True\" is set on content inside an ExtendedSplashScreen. " +
                "The region will defer its full initialization until services are available. " +
                "Navigation logging is temporarily disabled and will be enabled once the host starts.");
        }
        return _logger ?? NullLogger<NavigationRegion>.Instance;
    }
    set => _logger = value;
}

Documentation Changes:

  • Added a new section "Avoid Using Region.Attached in Shell.xaml Content" to doc/Learn/Navigation/HowTo-Regions.md that explains why Region.Attached="True" should not be used inside ExtendedSplashScreen content, shows incorrect usage with a code example marked to avoid, and provides the correct approach with examples showing where to define regions instead.
  • Added a concise IMPORTANT callout to doc/Learn/Navigation/Walkthrough/DefineRegions.md warning developers not to use Region.Attached="True" inside Shell.xaml or ExtendedSplashScreen content.

This documentation will be available to the Uno Docs MCP and LLM agents to prevent them from generating code with this anti-pattern.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Wasm UI Tests are not showing unexpected any differences. Validate PR Screenshots Compare Test Run results.
  • Contains NO breaking changes
  • Updated the Release Notes
  • Associated with an issue (GitHub or internal)

Other information

The documentation added to both HowTo-Regions.md and Walkthrough/DefineRegions.md serves as guidance for both developers and LLM/AI agents to avoid using Region.Attached in Shell.xaml content, which is an anti-pattern that can cause initialization issues.

Internal Issue (If applicable):

Original prompt

This section details on the original issue you should resolve

<issue_title>[Navigation] Logger exception is thrown when using custom ExtendedSplashScreen content with Region.Attached="True"</issue_title>
<issue_description>Using the Recommended Uno App Template where we have a Shell.xaml that includes an ExtendedSplashScreen will throw an error if we were to add our own custom Content on the ExtendedSplashScreen.

If we were to add a Grid with the Region.Attached Attached Property set to true like this:

<utu:ExtendedSplashScreen x:Name="Splash"
								  HorizontalAlignment="Stretch"
								  VerticalAlignment="Stretch"
								  HorizontalContentAlignment="Stretch"
								  VerticalContentAlignment="Stretch">
	<utu:ExtendedSplashScreen.LoadingContentTemplate>
		<DataTemplate>
			<Grid>
				<Grid.RowDefinitions>
					<RowDefinition Height="2*" />
					<RowDefinition />
				</Grid.RowDefinitions>

				<ProgressRing IsActive="True"
							  Grid.Row="1"
							  VerticalAlignment="Center"
							  HorizontalAlignment="Center"
							  Height="100"
							  Width="100" />
			</Grid>
		</DataTemplate>
	</utu:ExtendedSplashScreen.LoadingContentTemplate>

	<Grid uen:Region.Attached="True"
		  uen:Region.Navigator="Visibility">
		<!--  Content will be navigated here  -->
	</Grid>
</utu:ExtendedSplashScreen>

There is a runtime exception of type NullReferenceException with the Message: "Logger needs to be set"

The expectation is that the Navigation Extension handles the navigation from the Shell to the initial page by itself and so adding Content to the ExtendedSplashScreen is not needed. But we should still not crash if someone were to add this logic and we should provide a better warning/error message about proper usage. We should also document this in the documentation for Navigation
</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

…enceException when Region.Attached is used before NavigationHostedService starts

Co-authored-by: kazo0 <4793020+kazo0@users.noreply.github.com>
@CLAassistant

CLAassistant commented Nov 26, 2025

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ kazo0
❌ Copilot
You have signed the CLA already but the status is still pending? Let us recheck it.

Copilot AI changed the title [WIP] Fix logger exception for custom content in extended splash screen fix(Navigation): Use NullLogger fallback to prevent crash when Region.Attached is used in ExtendedSplashScreen Nov 26, 2025
Copilot AI requested a review from kazo0 November 26, 2025 15:41
@vatsashah45

Copy link
Copy Markdown
Contributor

@copilot This change doesn’t actually make the exception more descriptive, it removes the exception entirely. We should provide a better warning/error message about proper usage. By returning NullLogger<NavigationRegion>.Instance, the code will no longer throw when the logger isn’t initialized. That does fix the crash during early region attachment, but it also hides the underlying issue instead of surfacing a clearer diagnostic.

What I was expecting was either:

  • a more explicit exception (e.g., InvalidOperationException with a clearer message), or
  • a warning log explaining why we’re falling back to a null logger.

Right now, if something goes wrong later in the navigation pipeline, there won’t be any indication that the logger was never set.

Copilot AI and others added 2 commits November 26, 2025 16:54
…lization

Co-authored-by: vatsashah45 <72754751+vatsashah45@users.noreply.github.com>
Co-authored-by: vatsashah45 <72754751+vatsashah45@users.noreply.github.com>

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

@copilot This change doesn’t actually make the exception more descriptive, it removes the exception entirely. We should provide a better warning/error message about proper usage. By returning `NullL...

You're absolutely right. I've updated the fix to provide a clear diagnostic warning message when the logger isn't initialized (commit c4d3e26).

The solution now:

  • Outputs a warning to Console.Error explaining the situation and providing guidance about ExtendedSplashScreen usage
  • Uses Interlocked.CompareExchange to ensure the warning is only shown once even in multi-threaded scenarios
  • Still returns NullLogger<NavigationRegion>.Instance to prevent the crash

This gives developers visibility into what's happening while still allowing the application to run gracefully.

Copilot AI changed the title fix(Navigation): Use NullLogger fallback to prevent crash when Region.Attached is used in ExtendedSplashScreen fix(Navigation): Use NullLogger fallback with diagnostic warning to prevent crash when Region.Attached is used in ExtendedSplashScreen Nov 26, 2025
Copilot AI requested a review from vatsashah45 November 26, 2025 17:00
@kazo0

kazo0 commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

@copilot I was expecting you to get to the root of the problem as to why the exception is being thrown at the point of the runtime. To me, it seems to be a valid use-case to have a Grid with Region.Attached="True" as part of the Content of the ExtendedSplashScreen in the Shell. Upon inspection of this runtime exception and asking copilot why it was happening I was told: The problem is that when the XAML is parsed and Region.Attached="True" is encountered, it tries to initialize the navigation region, which needs the DI container to be ready - but it's not ready yet during the initial Shell construction.

With that information, can you find a better fix or a better way to log and document this usage as an error?

…d region initialization

Co-authored-by: kazo0 <4793020+kazo0@users.noreply.github.com>

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

@copilot I was expecting you to get to the root of the problem as to why the exception is being thrown at the point of the runtime. To me, it seems to be a valid use-case to have a Grid with Region.At...

After investigating the initialization flow, I found that the navigation system already properly defers region initialization. When Region.Attached="True" is encountered:

  1. A NavigationRegion is created and hooks up ViewLoading/ViewLoaded events
  2. When the view loads, AssignParent is called which calls FindServiceProvider()
  3. If services aren't available yet, it returns early (NavigationRegion.cs lines 237-244)
  4. Once services become available, the region properly initializes

The only issue was that Region.Logger was accessed before NavigationHostedService.StartAsync set it, causing the crash.

Using NullLogger as a fallback is the correct fix because:

  • The logging calls during early initialization are a no-op (expected)
  • The actual navigation work is properly deferred until services are ready
  • Once the host starts, normal logging resumes

Updated the diagnostic message to use Debug.WriteLine and clearly explain this deferred initialization behavior (commit 37bcca2).

@kazo0

kazo0 commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

@copilot We need to make sure that, when using LLMs to generate our app code, it does not add content to the Shell.xaml that is using Region.Attached. We should be adding something to the Navigation markdown documentation to mention avoiding this. the Uno Docs MCP should be able to provide that info to the LLM/agent

…content

Co-authored-by: kazo0 <4793020+kazo0@users.noreply.github.com>

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

@copilot We need to make sure that, when using LLMs to generate our app code, it does not add content to the Shell.xaml that is using Region.Attached. We should be adding something to the Navigation m...

Added documentation to doc/Learn/Navigation/HowTo-Regions.md with a new section "Avoid Using Region.Attached in Shell.xaml Content" that:

  • Explains why Region.Attached="True" should not be used inside ExtendedSplashScreen content
  • Shows incorrect usage with a code example marked to avoid
  • Explains that the navigation host is not ready during Shell construction
  • Provides the correct approach with examples showing where to define regions instead

This documentation will be available to the Uno Docs MCP and LLM agents (commit 55bf394).

Copilot AI changed the title fix(Navigation): Use NullLogger fallback with diagnostic warning to prevent crash when Region.Attached is used in ExtendedSplashScreen fix(Navigation): Use NullLogger fallback with diagnostic warning and add documentation to prevent crash when Region.Attached is used in ExtendedSplashScreen Nov 26, 2025
@kazo0

kazo0 commented Nov 26, 2025

Copy link
Copy Markdown
Contributor

@copilot You should add a concise version of these new documentation changes to the Walkthrough article located here: Navigation/Walkthrough/DefineRegions.md

Co-authored-by: kazo0 <4793020+kazo0@users.noreply.github.com>

Copilot AI commented Nov 26, 2025

Copy link
Copy Markdown
Contributor Author

@copilot You should add a concise version of these new documentation changes to the Walkthrough article located here: Navigation/Walkthrough/DefineRegions.md

Added a concise IMPORTANT callout to doc/Learn/Navigation/Walkthrough/DefineRegions.md near the top of the article warning developers not to use Region.Attached="True" inside Shell.xaml or ExtendedSplashScreen content (commit 19e52c9).

@unodevops

Copy link
Copy Markdown
Contributor

⚠️⚠️ The build 185212 has failed on uno.extensions.

@kazo0
kazo0 marked this pull request as ready for review November 27, 2025 04:35
@kazo0 kazo0 closed this Nov 27, 2025
@kazo0 kazo0 reopened this Nov 27, 2025
@kazo0
kazo0 enabled auto-merge November 27, 2025 14:49
@kazo0
kazo0 merged commit d75ce04 into main Nov 27, 2025
20 of 23 checks passed
@kazo0
kazo0 deleted the copilot/fix-logger-exception-splash-screen branch November 27, 2025 15:18
@kazo0

kazo0 commented Dec 1, 2025

Copy link
Copy Markdown
Contributor

@Mergifyio backport release/stable/7.0

@mergify

mergify Bot commented Dec 1, 2025

Copy link
Copy Markdown

backport release/stable/7.0

✅ Backports have been created

Details

kazo0 added a commit that referenced this pull request Dec 3, 2025
….0/pr-2958

fix(Navigation): Use NullLogger fallback with diagnostic warning and add documentation to prevent crash when Region.Attached is used in ExtendedSplashScreen (backport #2958)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Navigation] Logger exception is thrown when using custom ExtendedSplashScreen content with Region.Attached="True"

5 participants