[PR-Agent] Fix ApplyQueryAttributes called with empty dictionary on back

[kubaflo]

Note

Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!

Description

Fixes #33415

According to Microsoft's documentation, calling query.Clear() in ApplyQueryAttributes should prevent the method from being called again when re-navigating to the existing page (e.g., with GoToAsync("..")).

Current Behavior:
ApplyQueryAttributes IS called with an empty dictionary when navigating back, even after calling query.Clear().

Expected Behavior:
ApplyQueryAttributes should NOT be called with empty dictionary after query.Clear() has been called.

Root Cause

ApplyQueryAttributes was being invoked via two separate code paths during back navigation:

1. Path 1: When baseShellItem is ShellContent (ShellNavigationManager.cs, lines 313-331)
2. Path 2: When baseShellItem is NOT ShellContent (e.g., ShellSection) but isLastItem=true (lines 334-341)

Both paths would set/call ApplyQueryAttributes even when the merged query data was empty during back navigation.

Changes Made

Files Modified:

1. src/Controls/src/Core/Shell/ShellNavigationManager.cs (+12 lines)

   • Added check in Path 1 (ShellContent): Skip if mergedData.Count == 0 && isPopping == true
   • Added check in Path 2 (isLastItem): Skip setting property if mergedData.Count == 0 && isPopping == true

2. src/Controls/src/Core/Shell/ShellContent.cs (+3 lines)

   • Added early return when query is empty to prevent propagation

Logic:

// Skip applying/setting query attributes if empty and popping back
if (mergedData.Count > 0 || !isPopping)
{
    // Only call/set if data exists OR not popping
}

Tests Added:

• HostApp: src/Controls/tests/TestCases.HostApp/Issues/Issue33415.cs

  • TestShell-based test that navigates TO a page with parameters, then to another page, then back
  • MainPage implements IQueryAttributable and calls query.Clear()
  • Tracks call count to detect incorrect re-invocation

• NUnit: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33415.cs

  • Appium-based test validating the navigation flow
  • Verifies ApplyQueryAttributes called once (with params), not twice (with params + empty on back)

Verification

Scenario	Without Fix	With Fix
Navigate TO page with params	Call count: 1 ✅	Call count: 1 ✅
Navigate back from next page	Call count: 2 ❌	Call count: 1 ✅

✅ Tests FAIL without fix (bug reproduced)
✅ Tests PASS with fix (bug fixed)

Platforms Affected

• iOS
• Android
• (MacCatalyst and Windows likely affected but not mentioned in issue)

Breaking Changes

None - This restores the documented behavior.

[Copilot]

Pull request overview

This PR attempts to fix issue #33415 where ApplyQueryAttributes is incorrectly called with an empty dictionary when navigating back after query.Clear() has been called, which violates the documented behavior.

Key changes:

• Adds conditional checks in ShellNavigationManager.cs to skip applying/setting query attributes when the merged data is empty during back navigation
• Adds comprehensive UI test coverage (HostApp + NUnit) that reproduces the issue and validates the fix

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33415.cs	NUnit UI test that validates ApplyQueryAttributes is not called with empty dictionary on back navigation
src/Controls/tests/TestCases.HostApp/Issues/Issue33415.cs	TestShell-based test page with IQueryAttributable implementation that tracks call count and calls query.Clear()
src/Controls/src/Core/Shell/ShellNavigationManager.cs	Adds conditional logic to skip applying/setting query attributes when data is empty during back navigation

[Copilot]

The comments on lines 54-56 state "This assertion will FAIL (demonstrating the bug)" which is misleading since this test is part of the PR that includes the fix. The test should PASS with the fix applied. Consider updating the comment to reflect that this test verifies the fix works correctly, e.g., "This assertion verifies that ApplyQueryAttributes is NOT called when navigating back after query.Clear()"

Suggested change

	// BUG: According to documentation, after calling query.Clear() in ApplyQueryAttributes,
	// the method should NOT be called again when navigating back.
	// However, it IS called with an empty dictionary.
	var finalCallCount = App.FindElement("CallCountLabel").GetText();
	var finalStatus = App.FindElement("StatusLabel").GetText();
	// This assertion will FAIL (demonstrating the bug)
	// Expected: Call count should still be 1 (ApplyQueryAttributes should not be called on back)
	// Actual: Call count will be 2 (ApplyQueryAttributes IS called with empty dictionary)
	// According to documentation, after calling query.Clear() in ApplyQueryAttributes,
	// the method should NOT be called again when navigating back.
	// This test verifies that behavior and ensures ApplyQueryAttributes is not invoked with an empty dictionary.
	var finalCallCount = App.FindElement("CallCountLabel").GetText();
	var finalStatus = App.FindElement("StatusLabel").GetText();
	// This assertion verifies that ApplyQueryAttributes is NOT called when navigating back after query.Clear()
	// i.e., the call count should still be 1 when returning to the main page.

[Copilot]

The conditional check at lines 61-65 is redundant. If the assertion at line 57 passes (finalCallCount == "Call count: 1"), this code block will never execute. If the assertion fails, the test will already have failed. Consider removing this conditional block as it serves no purpose.

Suggested change

	// If the bug exists, this will show it was called with empty dictionary
	if (finalCallCount != "Call count: 1")
	{
	Assert.That(finalStatus, Is.EqualTo("Status: ApplyQueryAttributes called with EMPTY dictionary"),
	"If ApplyQueryAttributes is incorrectly called, it's called with empty dictionary");
	}

[PureWeen]

/rebase

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 33451

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 33451"

[kubaflo]

/rebase

[MauiBot]

🤖 AI Summary

📊 Expand Full Review — 59d3d1b · Update ShellNavigationManager.cs

---

🔍 Pre-Flight — Context & Validation

---

Issue: #33415 - ApplyQueryAttributes gets called with empty Dictionary on back
PR: #33451 - [PR-Agent] Fix ApplyQueryAttributes called with empty dictionary on back
Platforms Affected: iOS confirmed in issue validation; PR also claims Android may be affected
Files Changed: 1 implementation, 2 test

Key Findings

• The issue reports ApplyQueryAttributes being invoked again with an empty dictionary after query.Clear() and back navigation; the repro was validated on iOS.
• The actual PR changes are src/Controls/src/Core/Shell/ShellNavigationManager.cs plus a HostApp/UI test pair for Issue33415.
• The PR description mentions a ShellContent.cs change and dual-path fix, but the current PR file list only includes ShellNavigationManager.cs; that description appears stale.
• Inline review feedback flags one important correctness concern on ShellNavigationManager.cs: setting ShellContent.QueryAttributesProperty with empty merged data can itself trigger ApplyQueryAttributes.
• Existing review comments on the new UI test are mostly about cleanup/misleading comments rather than functional coverage.
• No prior PRAgent review comment was found to resume from.

Edge Cases Mentioned

• User expectation is specifically that query.Clear() prevents a later back-navigation callback, not merely that old parameters are removed.
• The issue body calls out both Dictionary<string, object> and ShellNavigationQueryParameters as relevant navigation payload shapes.
• The user notes a workaround of early-returning in ApplyQueryAttributes when the query is empty, but also questions whether the framework should always invoke the method regardless of argument presence.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #33451	Skip applying/setting merged query attributes during pop navigation when merged data is empty in ShellNavigationManager.cs; cover with Issue33415 HostApp + UI PENDING (Gate)	src/Controls/src/Core/Shell/ShellNavigationManager.cs, src/Controls/tests/TestCases.HostApp/Issues/Issue33415.cs, src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue33415.cs	PR description also references ShellContent.cs, but that file is not in the current PR diff	test

---

🚦 Gate — Test Verification

---

Gate Result PASSED:

Platform: ios
Mode: Full Verification

• Tests FAIL without fix:
• Tests PASS with fix:

Evidence: Verification was run through the required isolated flow for Issue33415 on iOS. The gate run reported the expected two-way result: failing without the PR fix and passing with the PR fix.

---

🔧 Fix — Analysis & Comparison

---

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
empty transitions
2	try-fix / claude-sonnet-4.6	Override equality so empty instances compare equal and do not trigger property-changed callbacks	PASS	1 file	Passed, but changes global equality semantics for ShellRouteParameters, increasing blast radius
3	try-fix / gpt-5.3-codex	Preserve the existing empty query instance in ShellNavigationManager.MergeData during pop FAIL	1 file	Could not be behaviorally validated because the required iOS test command failed during unrelated ImageMagick.Drawing compilation	navigation
4	try-fix / gemini-3-pro-preview	Add a receiver-side guard in ShellContent.ApplyQueryAttributes for semantically empty old/new FAIL	1 file	Subagent response dropped; recovered test output shows the same unrelated ImageMagick.Drawing compile failure	values
PR	PR #33451	Skip applying/setting merged query attributes during pop navigation when merged data is empty PASSED (Gate)	3 files	Original PR validated on iOS gate; most targeted change for the documented back-navigation bug

Cross-Pollination

Model	Round	New Ideas?	Details
claude-opus-4.6	2	No	NO NEW IDEAS
claude-sonnet-4.6	2	No	NO NEW IDEAS
gpt-5.3-codex	2	No	NO NEW IDEAS
gemini-3-pro-preview	2	No	NO NEW IDEAS

Exhausted: Yes
Selected Fix: PR's it already passed Gate on iOS and is the narrowest, most behavior-specific solution. The passing alternatives were technically viable but either broadened receiver-side semantics (ShellContent) or altered global equality behavior (ShellRouteParameters).fix

---

📋 Report — Final Recommendation

---

Final Recommendation: APPROVE

Phase Status

Phase	Status	Notes
Pre-Flight COMPLETE	Linked issue, PR context, review comments, and changed-file classification captured
Gate PASSED	iOS full verification reported FAIL without fix and PASS with fix for Issue33415
Try-Fix COMPLETE	4 required model attempts completed, 2 passing alternatives, no new ideas in cross-pollination
Report COMPLETE

Summary

PR #33451 addresses issue #33415 by preventing empty merged Shell query data from being re-applied during back navigation after a page has already called query.Clear(). The saved gate result shows the added test catches the bug without the fix and passes with the fix on iOS. Try-fix exploration found alternative solutions, but the PR's implementation remained the best choice because it is the most targeted and least invasive.

Root Cause

During back navigation, Shell was still producing and re-applying an empty ShellRouteParameters payload after single-use query data had been cleared. That empty payload flowed back into ApplyQueryAttributes, causing a second callback with an empty dictionary even though the documented behavior is that cleared query data should not be re-delivered.

Fix Quality

The PR fixes the problem at the navigation source by skipping apply/set operations only for the specific pop-navigation + empty-merged-data case. Compared with the passing alternatives, this avoids broad receiver-side behavior changes in ShellContent and avoids changing global equality semantics in ShellRouteParameters. One non-blocking documentation nit remains from pre-flight: the PR description references a ShellContent.cs change that is not present in the current diff.

---