Bridge DI-registered Essentials implementations to static facades - #35068
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 35068Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 35068" |
f4ea0a1 to
a48a401
Compare
🔍 Skill Validation Results✅ Static Checks PassedSkills checked: 15 | Agents checked: 3 Full validator output❌ LLM Evaluation Failed0/4 skill(s) passed validation
❌ code-review: Improvement score 6.4% below threshold of 10.0% (not statistically significant) [high variance in: Happy path - code review PR request, Independence-first - agent reads diff before description, Verdict consistency - errors must map to NEEDS_CHANGES] [NOT ACTIVATED (isolated) in 2 scenario(s)] [NOT ACTIVATED (plugin) in 2 scenario(s)] ❌ evaluate-pr-tests: Improvement score 13.1% meets threshold of 10.0% [high variance in: Happy path - evaluate PR tests and produce structured report, Negative trigger - general code review should not produce test evaluation report, Anti-pattern detection - Thread.Sleep and obsolete APIs, Test type downgrade recommendation - UI test for pure property logic, Weak assertion detection - meaningless test assertions, Edge case gaps analysis - fix with multiple branches untested, No tests added - PR only has fix files, Fluent chain wait pattern should not trigger missing-wait warning] [NOT ACTIVATED (isolated) in 2 scenario(s)] [NOT ACTIVATED (plugin) in 3 scenario(s)] ❌ try-fix: Eval scenario 'Regression: agent must not claim success without running the test command' prompt mentions target name 'try-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Regression: agent uses prescribed restore script, not raw git commands' prompt mentions target name 'try-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Edge case: exhausted iterations produces documented Fail, not silence or Pass' prompt mentions target name 'try-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. ❌ verify-tests-fail-without-fix: Eval scenario 'Regression: tests passing without fix means verification FAILED' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Edge case: no test files detected in the PR' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. Eval scenario 'Regression: agent correctly reports test failure as verification success' prompt mentions target name 'verify-tests-fail-without-fix' (skill or agent) — remove the target name from the prompt to avoid biasing baseline runs. [1] High run-to-run variance (CV=158.33). Timeout at 300s
📊 code-review / anti-pattern-never-approve-via-github-apiJudge Report: Anti-pattern - never approve via GitHub APIBaseline JudgeOverall Score: 5/5
With-Skill Judge (Isolated)Overall Score: 5/5
With-Skill Judge (Plugin)Overall Score: 5/5
Baseline Agent Output
PR #28713 Review — Already MergedThis PR is already merged, so approval is moot. Also, per repository policy, I cannot approve PRs — approval is a human decision. Review SummaryThis is a large integration PR bundling 7 independent changes: ✅ Core Fix: iOS Shell Flyout Header/Footer Resize
✅ FlyoutPage Memory Leak Fix
✅ XAML SourceGen: x:Key Escape Fix (#34726)
✅ XAML SourceGen: Converter+x:DataType regression (#34713)
|
| File | Change |
|---|---|
Label.Mapper.cs |
Guard in MapLineHeight, MapTextDecorations, MapCharacterSpacing changed from !IsPlainText(label) → label.HasFormattedTextSpans |
Label.iOS.cs |
MapFormatting now also triggers LineHeight, TextDecorations, CharacterSpacing updates for HTML labels |
Issue22197.* |
UI test + host app page comparing HTML vs plain text label styling |
Maui34713.* |
XAML unit tests for source-gen converter resolution (separate issue #34713) |
| Snapshots | Android, iOS, Mac, Windows reference images |
Core Fix Analysis ✅
The bug: IsPlainText() returns false for both formatted spans and HTML text. The mapper methods were early-returning for HTML labels, preventing LineHeight, TextDecorations, and CharacterSpacing from being applied.
The fix correctly narrows the guard to HasFormattedTextSpans only — formatted spans should skip these (spans have their own styling), but HTML labels should receive them. The iOS MapFormatting change ensures these properties are refreshed when TextType changes to HTML.
The control flow is consistent: MapFormatting triggers UpdateValue(LineHeight) → mapper's MapLineHeight checks HasFormattedTextSpans (false for HTML) → proceeds to baseMethod. ✅
Issues Found
🟡 Medium — Mixed concerns in single PR
The PR bundles two unrelated fixes: Issue #22197 (HTML label properties) and Issue #34713 (XAML source gen converter resolution via Maui34713.* files). These should ideally be separate PRs for cleaner history and easier revert if needed.
🟡 Low — Missing trailing newlines
Issue22197.xaml, Issue22197.xaml.cs, and Issue22197.cs all lack trailing newlines (\ No newline at end of file). Minor but inconsistent with typical conventions.
🟢 No bugs found — The mapper guard change is logically sound. The IsPlainText() method is still used in Label.iOS.cs MapFormatting, so it's not dead code. The fix is appropriately scoped to #if ANDROID || IOS platforms.
Verdict
Approve — The core fix is correct and well-tested with cross-platform snapshot tests. The only notable concern is the unrelated Maui34713 test files bundled into the same PR.
`
With-Skill Agent Output (Isolated)
`
Here's my complete code review:
Code Review — PR #31202
Independent Assessment
What this changes: This is a multi-part PR bundling 4 distinct changes:
-
Label HTML fix: Changes the guard in
MapLineHeight,MapTextDecorations, andMapCharacterSpacingfrom!IsPlainText(label)(which blocked both formatted-text-with-spans AND HTML) tolabel.HasFormattedTextSpans(which only blocks formatted-text-with-spans). This allows these 3 properties to apply to HTML labels. On iOS,MapFormattingis also updated to re-trigger these properties after HTML text is set. -
XAML source gen tests (Maui34713): Regression tests verifying the source generator correctly handles bindings with
Converter={StaticResource}insidex:DataTypescopes — covering compile-time resolution (page resources) and runtime resolution (app resources). -
gh-aw CI workflow: New agentic workflow (
copilot-evaluate-tests) for automated test quality evaluation on PRs, with fork-PR security model. -
Merge flow: Arcade inter-branch merge from
net11.0→release/11.0.1xx-preview3.
Inferred motivation: Issue #22197 reports that LineHeight doesn't work on HTML labels. The old IsPlainText check was overly broad — it blocked properties for all non-plain-text (including HTML), when the intent was only to skip them for formatted text with explicit Span objects.
Reconciliation with PR Narrative
Author claims: Bug fix for HTML labels not respecting LineHeight/TextDecorations/CharacterSpacing, plus CI infrastructure additions.
Agreement: My independent assessment aligns with the author's description. The root cause analysis is correct — IsPlainText conflated "has spans" with "is HTML", and the fix correctly distinguishes the two cases.
Findings
⚠️ Warning — Stale comments contradict new behavior
Two comments in Label.Mapper.cs (lines 38 and 114) still say:
csharp // these are for properties that should only apply to plain text (not spans nor html)
After this PR, these properties apply to plain text AND HTML — only spans are excluded. The comments should read something like:
csharp // these are for properties that should not apply to formatted text with spans
⚠️ Warning — Known test regression was not caught before merge
PR #34934 was needed as a follow-up to fix the CharacterSpacingWithLineHeightWithTextDecorationsWorksCorrectly device test, whose expectations for label4 (HTML label) still expected (0, 0, None) instead of the now-correct (5, 1.5, Underline). This indicates the existing device test suite wasn't fully run or verified before merge.
💡 Suggestion — IsPlainText is now only used by iOS
After this change, IsPlainText() in Label.Mapper.cs is only called from Label.iOS.cs's MapFormatting. It could be moved to the iOS-specific file or marked as iOS-only to reduce confusion about its scope.
💡 Suggestion — Android has no MapFormatting equivalent
On iOS, MapFormatting re-triggers LineHeight/TextDecorations/CharacterSpacing after HTML text is set (because setting NSAttributedString can reset platform properties). Android's Label.Android.cs has no equivalent re-application — it relies on the mapper running these properties independently. This appears to work because Android's TextView maintains lineSpacingMultiplier/letterSpacing independently of text content, but it's worth verifying on Android device tests.
💡 Suggestion — Gather-TestContext.ps1 markdown injection fix is good
The Escape-ForCodeSpan function and double-backtick code spans prevent markdown injection via crafted filenames. The path traversal guard for API-downloaded files (StartsWith check) is also a solid security measure.
Devil's Advocate
-
Could the Label change break existing apps? Apps with HTML labels that previously had
LineHeight/TextDecorations/CharacterSpacingset but silently ignored will now see those properties applied. This is an intentional behavior change fixing a bug, but could surprise users who accidentally set these properties. The risk is low — the new behavior is correct. -
Am I sure the stale comments matter? Yes — they directly contradict the code's behavior and would mislead future contributors into thinking HTML labels are still excluded.
-
Is the test regression actually a problem? It was caught and fixed via [Testing] Fix Label CharacterSpacing/LineHeight/TextDecorations test for HTML labels #34934. Ideally the existing test would have been updated in this PR, but the follow-up was quick and the fix is straightforward.
Verdict: NEEDS_DISCUSSION
Confidence: medium
Summary: The core Label fix is correct and well-reasoned — HasFormattedTextSpans is the right guard for these properties. However, the stale comments (lines 38 and 114 of Label.Mapper.cs) actively contradict the new behavior and should be updated. The test regression that required a follow-up PR (#34934) suggests the device test suite wasn't fully validated pre-merge. These are not blocking issues, but the comments should be corrected to prevent future misunderstanding.
`
With-Skill Agent Output (Plugin)
Now I have both versions. Let me read more review rules and check Android handling.
📊 code-review / negative-trigger-describe-changes-query
Judge Report: Negative trigger - describe changes query
Baseline Judge
Overall Score: 5/5
Reasoning: The agent efficiently handled the task in just 3 tool calls and 18.8 seconds. It gracefully recovered from the gh CLI authentication failure by falling back to web_fetch. The final summary is concise, accurate, and well-organized — covering the new APIs, the rationale (Hot Reload in-place mutation), implementation approach (unapply/reapply), and relevant metadata (EditorBrowsable, linked issues, target branch). No review workflow was triggered and no severity markers appear, fully meeting both rubric criteria.
- The agent provides a descriptive summary without triggering the full review workflow: 5/5 — The agent provided a clear, concise summary of PR Add InvalidateStyle() and VisualStateManager.InvalidateVisualStates() APIs #34723 explaining the new APIs added, the motivation (Hot Reload scenarios), and the approach (unapply then reapply styles/visual states). It did not trigger any code review workflow — it simply fetched the PR page and summarized it. The approach was efficient: tried gh CLI first, failed due to no token, then fell back to web_fetch, which worked. The summary is well-structured and directly answers the user's question about understanding the approach.
- No severity markers (❌/
⚠️ /💡) or verdicts appear in the output: 5/5 — The output contains no severity markers or verdict language. It is a clean descriptive summary without any review-style annotations.
With-Skill Judge (Isolated)
Overall Score: 4.7/5
Reasoning: The agent produced a high-quality, well-structured summary that directly answers the user's question about understanding the approach of PR #34723. The summary is accurate (matching the PR title/description from the web fetch), concise, and appropriately scoped. Minor inefficiency in the execution path — the agent tried the gh CLI three times before falling back to web_fetch, wasting a few turns — but it recovered cleanly and the final output is excellent. The approach of fetching from the web and summarizing without over-engineering (no code review agent, no deep file analysis) was the right call for this user request.
- The agent provides a descriptive summary without triggering the full review workflow: 5/5 — The agent provided a clear, concise summary of PR Add InvalidateStyle() and VisualStateManager.InvalidateVisualStates() APIs #34723 explaining the problem (in-place mutation of styles/visual states not triggering UI updates during Hot Reload), the approach (new invalidation APIs), and the specific APIs added. It did not trigger any code review workflow — it simply fetched the PR page and summarized the content. The approach was efficient: one failed gh CLI attempt, then a successful web_fetch, producing a well-structured summary.
- No severity markers (❌/
⚠️ /💡) or verdicts appear in the output: 5/5 — The output contains no severity markers or verdict language. It is purely a descriptive summary of the PR's purpose and approach, with no review-style judgments, ratings, or emoji markers.
With-Skill Judge (Plugin)
Overall Score: 5/5
Reasoning: The agent correctly identified this as a summary request rather than a review request. Despite encountering authentication issues with the GitHub CLI (3 failed attempts), it efficiently recovered by using web_fetch to retrieve the PR page. The final summary is accurate, well-organized, and directly addresses the user's goal of understanding the approach. The output is clean with no review artifacts, severity markers, or verdicts. The path was slightly inefficient (4 bash calls before falling back to web_fetch), but the end result fully meets the user's needs.
- The agent provides a descriptive summary without triggering the full review workflow: 5/5 — The agent correctly identified this as a summarization request, not a code review. It fetched the PR details via web_fetch and produced a clear, concise summary of the PR's problem statement and approach — covering the four new APIs, their purposes, and the EditorBrowsable attribute. No review workflow was triggered; no code was analyzed line-by-line for issues.
- No severity markers (❌/
⚠️ /💡) or verdicts appear in the output: 5/5 — The output contains zero severity markers or verdict language. It is purely a descriptive summary with bold headings for structure, but no review-style annotations.
Baseline Agent Output
`
PR #34723 adds public APIs to force reapplication of styles and visual states that have been mutated in-place — primarily to support Hot Reload scenarios.
New APIs:
StyleableElement.InvalidateStyle()— unapplies then reapplies the merged styleSpan.InvalidateStyle()/ImageSource.InvalidateStyle()— same, for types that own their ownMergedStyleVisualStateManager.InvalidateVisualStates(VisualElement)— unapplies then reapplies current visual state setters across all groups
Why:
When Hot Reload mutates a style or visual state in place (without replacing the object), the UI won't update because MAUI doesn't know anything changed. These APIs let the Hot Reload infrastructure explicitly tell MAUI "re-evaluate this style/visual state now."
The methods are marked with [EditorBrowsable(Never)] so they don't clutter IntelliSense for app developers. Fixes issues #34721 and #34722. Merged into the net11.0 branch (shipped in .NET 11 Preview 3).
`
With-Skill Agent Output (Isolated)
`
Here's a summary of PR #34723:
Goal: Add public APIs to force reapplication of in-place-mutated styles and visual states, primarily for Hot Reload scenarios.
Approach: Introduces four new Invalidate* methods:
StyleableElement.InvalidateStyle()— forces unapply + reapply of the element's merged styleSpan.InvalidateStyle()— same, since Span owns its ownMergedStyleImageSource.InvalidateStyle()— same forImageSourceVisualStateManager.InvalidateVisualStates(VisualElement)— forces unapply + reapply of the current visual state setters across all groups
Why: When styles or visual states are mutated in place (e.g., during Hot Reload), MAUI doesn't automatically detect the change. These APIs let callers explicitly trigger a re-evaluation cycle (unapply → reapply) so updated values take effect.
The APIs are marked to be hidden from IntelliSense since they're intended for infrastructure use, not typical app developers. The PR targets the net11.0 branch (.NET 11 Preview 3).
`
With-Skill Agent Output (Plugin)
`
Here's what PR #34723 does:
Goal: Add public APIs to force reapplication of in-place-mutated styles and visual states, primarily for Hot Reload scenarios.
New APIs added:
StyleableElement.InvalidateStyle()— forces unapply + reapply of the merged styleSpan.InvalidateStyle()— same, since Span owns its own MergedStyleImageSource.InvalidateStyle()— same for ImageSourceVisualStateManager.InvalidateVisualStates(VisualElement)— forces unapply + reapply of current visual state setters across all groups
Approach: These are caller-driven invalidation methods. When a style or visual state is mutated in-place (e.g., during Hot Reload), the framework normally doesn't know anything changed. These APIs let the caller explicitly trigger a re-evaluation cycle (unapply then reapply), so the UI reflects the updated values.
The APIs are marked with [EditorBrowsable(Never)] so they don't clutter IntelliSense for app developers — they're infrastructure-level, intended for the Hot Reload pipeline.
Fixes: Issues #34721 and #34722. Merged and shipped in .NET 11 Preview 3.
`
📊 code-review / negative-trigger-informational-query-about-a-pr
Judge Report: Negative trigger - informational query about a PR
Baseline Judge
Overall Score: 5/5
Reasoning: The agent performed the task efficiently and correctly. It recovered gracefully from the gh CLI authentication failure by falling back to the public GitHub API, extracted the relevant information, and delivered a clear, accurate, well-structured summary of the PR. The summary includes the purpose (fixing unescaped special characters in x:Key values), the specific files changed, and the PR status — all without unnecessary steps or overhead. This is exactly the kind of lightweight, direct response the task called for.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent took a straightforward approach: attempted gh CLI (failed due to missing token), fell back to web_fetch, then produced a clean summary. No multi-step review workflow, no elaborate analysis pipeline—just a direct summary of the PR as requested.
- The agent does NOT load review-rules.md or walk through MAUI-specific review checklists: 5/5 — The agent did not attempt to load any review-rules.md file, nor did it walk through any MAUI-specific review checklists. It simply fetched the PR page and summarized it.
With-Skill Judge (Isolated)
Overall Score: 4.3/5
Reasoning: The agent handled this simple summarization task well. It encountered some friction with GitHub CLI authentication (trying multiple token approaches), but recovered quickly by falling back to web_fetch. The final summary is accurate, well-structured, and appropriately scoped—covering the title, problem, fix, linked issue, status, and author. It did not over-engineer the response with unnecessary review workflows or checklists, matching both rubric criteria perfectly.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent simply fetched the PR page via web_fetch and produced a concise, plain-language summary. No structured multi-step review workflow was launched — no sub-agents, no checklists, no formal review process. It was a straightforward fetch-and-summarize approach.
- The agent does NOT load review-rules.md or walk through MAUI-specific review checklists: 5/5 — The agent never attempted to load any review-rules.md file or any MAUI-specific review checklists. It focused entirely on reading the PR content from GitHub and summarizing it.
With-Skill Judge (Plugin)
Overall Score: 4.3/5
Reasoning: The agent efficiently handled a simple summarization request. After reasonable troubleshooting of GitHub CLI token issues (3 attempts), it pivoted to web_fetch and retrieved the PR page and diff in 2 calls. The final summary is accurate, well-structured, and appropriately concise—covering the title, problem, fix details, status, and linked issue. No unnecessary overhead or over-engineering was introduced. The approach was methodical and the output directly answers the user's question.
- The agent provides a plain summary without launching a structured multi-step review workflow: 5/5 — The agent simply fetched the PR page via web_fetch and produced a concise summary. No structured multi-step review workflow was launched — no sub-agents, no checklists, no formal review phases.
- The agent does NOT load review-rules.md or walk through MAUI-specific review checklists: 5/5 — The agent never searched for, loaded, or referenced any review-rules.md file or MAUI-specific review checklists. It stayed focused on summarizing the PR content.
Baseline Agent Output
**PR #34727** fixes a bug where the XAML source generator didn't escape special characters (quotes, backslashes, control chars) inx:Key` values when emitting C# string literals, causing compilation errors.
Changes:
SetPropertyHelpers.cs&KnownMarkups.cs— Escapex:Keyvalues before interpolating into generated code.CSharpExpressionHelpers.cs— MadeEscapeForStringinternalso it can be reused across files.- Added a XAML unit test (
Maui34726) verifying keys with quotes/backslashes work across source-gen, runtime, and XamlC paths.
5 files changed, +140 / −71. The PR is closed (merged).
`
With-Skill Agent Output (Isolated)
`
Here's a summary of PR #34727:
Title: Escape x:Key values in XAML source generation
Problem: When x:Key values in XAML contained special characters (double quotes, backslashes, or control characters), the XAML source generator (SetPropertyHelpers.AddToResourceDictionary) would produce syntactically invalid C# code because those characters weren't escaped.
Fix: Uses CSharpExpressionHelpers.EscapeForString() to properly escape x:Key values before interpolating them into generated C# code.
Fixes: Issue #34726
Status: Closed/Merged
Author: @StephaneDelcroix (co-authored with Copilot)
A follow-up commit by @PureWeen also addressed a second missed escape path in AddFactory("...").
`
With-Skill Agent Output (Plugin)
`
Here's a summary of PR #34727:
Title: Escape x:Key values in XAML source generation
Problem: When x:Key values in XAML contained special characters (double quotes, backslashes, control characters), the XAML source generator would produce syntactically invalid C# code.
Fix:
- Changed
EscapeForString()inCSharpExpressionHelpersfromstatictointernal staticso it can be called from other classes - Used
EscapeForString()to properly escapex:Keyvalues before interpolating them into generated C# code inSetPropertyHelpers.AddToResourceDictionary - Also included whitespace/formatting cleanups across the file
Status: Closed (superseded — a follow-up commit by PureWeen fixed an additional missed escape path in AddFactory()).
Fixes: Issue #34726
`
📊 code-review / verdict-consistency-errors-must-map-to-needs-changes
Judge Report: Verdict consistency - errors must map to NEEDS_CHANGES
Baseline Judge
Overall Score: 3.7/5
Reasoning: The agent produced a well-organized and technically sound review that correctly identified the main ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs, along with two minor issues. The systematic approach of checking all handler files for symmetry was thorough. However, two notable gaps prevent a higher score: (1) no explicit NEEDS_CHANGES verdict despite finding a bug, and (2) no line number references. The agent was efficient in its approach (recovering from gh CLI failure, methodically chunking the large diff) and the analysis quality is solid, but the output falls short of a complete, actionable code review due to the missing verdict and line citations.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 3.7/5 — The agent clearly identified a bug (labeled with 🐛) — the UnregisterView vs RemoveViewWithLocalListener mismatch — and presented it as something that needs to be fixed, with a specific code change suggestion. The entire output is structured as 'here are problems that need addressing,' which implicitly communicates NEEDS_CHANGES. However, it never explicitly states a verdict like 'NEEDS_CHANGES' or 'Request Changes'. A small deduction for not making the verdict unambiguously explicit.
- The agent applies handler lifecycle rules from review-rules.md (ConnectHandler/DisconnectHandler symmetry): 4.7/5 — The agent thoroughly analyzed ConnectHandler/DisconnectHandler symmetry across all changed handler files. It built a comprehensive comparison table showing which files correctly pair SetupViewWithLocalListener with RemoveViewWithLocalListener and which don't. It explained exactly what SetupViewWithLocalListener does (3 steps) and how UnregisterView only handles 1 of those steps. It also caught a secondary asymmetry with RegisterParentForChildViews/UnregisterView for the DrawerLayout platformView. This is exactly the kind of lifecycle symmetry analysis requested.
- The agent cites specific file and line references for the concern: 3.3/5 — The agent cites specific filenames (FlyoutViewHandler.Android.cs, ShellContentFragment, ShellFlyoutTemplatedContentRenderer, ShellSectionRenderer, NavigationRootManager), specific method names (SetupViewWithLocalListener, RemoveViewWithLocalListener, UnregisterView, RegisterParentForChildViews), and provides exact code snippets for both the problematic code and the suggested fix. It does not cite specific line numbers (e.g., 'line 142'), though the file and method references are precise enough to locate the issues unambiguously.
With-Skill Judge (Isolated)
Overall Score: 4.3/5
Reasoning: The agent produced a high-quality technical review despite significant infrastructure challenges (GitHub API rate limits, no auth token, large diff requiring creative splitting). It correctly identified the ConnectHandler/DisconnectHandler asymmetry the user hinted at, provided detailed technical analysis of the impact (memory leaks from orphaned ViewCompat listeners, broken reconnection from premature nulling), and cross-validated against other files in the PR. The review demonstrates genuine understanding of Android handler lifecycle patterns in MAUI. The main weaknesses are: (1) no explicit NEEDS_CHANGES verdict, (2) the output was truncated at the end due to timeout — the last suggestion about static _registeredViews is incomplete, and (3) line numbers are not cited. Despite these issues, the substantive technical analysis is excellent and actionable.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 4/5 — The agent found a clear ❌ Error-level issue (ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs where UnregisterView is used instead of RemoveViewWithLocalListener) and correctly assigned a verdict of NEEDS_CHANGES with high confidence. The verdict is fully consistent with the finding.
- The agent applies handler lifecycle rules from review-rules.md (ConnectHandler/DisconnectHandler symmetry): 5/5 — The agent explicitly read review-rules.md (sections 1-250+), referenced §1 Handler Lifecycle & Patterns, and quoted it as the '[Draft] Readme WIP #1 source of handler leaks'. The analysis correctly identifies the three-step setup in SetupViewWithLocalListener (SetOnApplyWindowInsetsListener, SetWindowInsetsAnimationCallback, RegisterView) vs the single-step teardown in UnregisterView, demonstrating precise application of the symmetry rule. The cross-referencing of all other teardown sites (NavigationRootManager, ShellContentFragment, ShellFlyoutTemplatedContentRenderer, ShellSectionRenderer) that correctly use RemoveViewWithLocalListener strengthens the argument using the codebase's own patterns.
- The agent cites specific file and line references for the concern: 3.7/5 — The agent cites specific files (FlyoutViewHandler.Android.cs, ShellSectionRenderer.cs, MauiPageControl.cs), approximate line numbers (~288-293, ~304-309), specific method names (SetupViewWithLocalListener, UnregisterView, RemoveViewWithLocalListener), and inline code snippets. The line numbers are approximate (prefixed with ~) rather than exact, which is a minor shortcoming but understandable given the agent was working from a diff. Cross-references to four other files with their specific teardown methods are also provided. Overall the references are specific enough to immediately locate every concern.
With-Skill Judge (Plugin)
Overall Score: 4.7/5
Reasoning: The agent produced an excellent, thorough code review despite authentication issues with GitHub CLI (gracefully fell back to web_fetch). It methodically gathered the full diff (paginating through 80000+ characters), read the review rules, verified findings against actual source code, and produced a well-structured output with three genuine findings of varying severity. The main finding (ConnectHandler/DisconnectHandler asymmetry) is well-reasoned and backed by evidence from other teardown sites. The devil's advocate section demonstrates critical thinking. The review is actionable with a clear fix suggestion. 23 tool calls over ~158 seconds is reasonable for this complexity.
- If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM: 5/5 — The agent clearly identified the FlyoutViewHandler.Android.cs ConnectHandler/DisconnectHandler asymmetry as a ❌ Error-level issue and gave the verdict NEEDS_CHANGES with high confidence. The agent also applied devil's advocate reasoning to confirm the issue is genuine (not intentional), noting that other Shell handlers all use RemoveViewWithLocalListener, and that the CoordinatorLayout can be reattached making orphaned listeners a real problem.
- The agent applies handler lifecycle rules from review-rules.md (ConnectHandler/DisconnectHandler symmetry): 5/5 — The agent read review-rules.md (lines 1-80, which covers 'Handler Lifecycle & Patterns') and directly applied the ConnectHandler/DisconnectHandler symmetry rule. It traced the exact setup path (SetupViewWithLocalListener → ViewCompat.SetOnApplyWindowInsetsListener + SetWindowInsetsAnimationCallback + RegisterView) and compared it to the teardown path (only UnregisterView), identifying the three missing cleanup operations. It further validated by cross-referencing all other disconnect sites in the PR (ShellContentFragment.Destroy, ShellSectionRenderer.Destroy, ShellFlyoutTemplatedContentRenderer.Disconnect, NavigationRootManager.Disconnect) which all correctly use RemoveViewWithLocalListener.
- The agent cites specific file and line references for the concern: 4/5 — The agent cites specific file names (FlyoutViewHandler.Android.cs, MauiPageControl.cs, ShellSectionRenderer.cs, MauiWindowInsetListener.cs) and specific method names (ConnectHandler, DisconnectHandler, Destroy, Disconnect). It includes code snippets showing the asymmetry and lists the exact API calls involved (SetupViewWithLocalListener, UnregisterView, RemoveViewWithLocalListener). However, no explicit line numbers are provided. The agent was constrained by the fact that the PR branch wasn't checked out locally (files didn't exist on disk), and it worked purely from the diff fetched via web_fetch. While file + method references are actionable, explicit line numbers from the diff would have made the review more precise.
Baseline Agent Output
`
Here's my analysis of the ConnectHandler/DisconnectHandler patterns across all handler files changed in this PR:
🔴 FlyoutViewHandler.Android.cs — Asymmetric teardown (Bug)
This is the real issue. In ConnectHandler, the CoordinatorLayout is set up with the full SetupViewWithLocalListener:
csharp // ConnectHandler MauiWindowInsetListener.SetupViewWithLocalListener(cl); // → attaches ViewCompat.SetOnApplyWindowInsetsListener // → attaches ViewCompat.SetWindowInsetsAnimationCallback // → registers in static registry
But DisconnectHandler uses only UnregisterView:
csharp // DisconnectHandler MauiWindowInsetListener.UnregisterView(cl); // ← incomplete
This skips clearing the ViewCompat listeners and resetting applied safe areas. Every other handler in this PR uses the symmetric RemoveViewWithLocalListener — e.g., ShellContentFragment, ShellFlyoutTemplatedContentRenderer, ShellSectionRenderer, NavigationRootManager all do it correctly. The fix should be:
csharp MauiWindowInsetListener.RemoveViewWithLocalListener(cl);
🟡 ShellSectionRenderer.cs — Redundant null check (Minor)
csharp if (_rootView != null) // outer check { if (_rootView is not null) // redundant inner check { MauiWindowInsetListener.RemoveViewWithLocalListener(_rootView); }
The inner is not null is always true at that point. Harmless but noisy.
🟡 MauiPageControl.cs — Orphaned pragma (Minor)
The #pragma warning disable CA1416 was removed along with the if block, but #pragma warning restore CA1416 remains on the next line — creating an unmatched restore directive.
✅ All other handlers are symmetric
- ShellContentFragment:
SetupViewWithLocalListener↔RemoveViewWithLocalListener✅ - ShellFlyoutTemplatedContentRenderer:
SetupViewWithLocalListener↔RemoveViewWithLocalListener✅ - NavigationRootManager:
SetupViewWithLocalListener↔RemoveViewWithLocalListener✅ - ToolbarHandler:
TrySetMauiWindowInsetListener↔RemoveMauiWindowInsetListener✅ - ContentViewGroup/LayoutViewGroup/MauiScrollView: Attach on
OnAttachedToWindow, detach onOnDetachedFromWindow✅
With-Skill Agent Output (Isolated)
`
I now have all the context I need. Let me compile the review.## Code Review — PR #32278
Independent Assessment
What this changes: Replaces the global, per-activity GlobalWindowInsetListener with a per-view static registry (MauiWindowInsetListener). Each CoordinatorLayout now gets its own local listener, and child views discover it by walking the view tree via FindListenerForView. The old GlobalWindowInsetListener.cs is deleted; MauiAppCompatActivity no longer holds a listener instance.
Inferred motivation: The global listener model broke in complex navigation (modals, nested flyouts) where multiple CoordinatorLayouts competed for a single listener instance, causing cross-contamination of inset state.
Reconciliation with PR Narrative
Author claims: "[Android] Refactor WindowInsetListener to per-CoordinatorLayout registry with optimized lookups"
Agreement: The code matches this description. The refactor is consistent and well-applied across ~30 files — with one notable exception.
Findings
❌ Error — FlyoutViewHandler.DisconnectHandler uses UnregisterView instead of RemoveViewWithLocalListener for the CoordinatorLayout
File: src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs
ConnectHandler sets up the CoordinatorLayout with the full 3-step setup:
csharp MauiWindowInsetListener.SetupViewWithLocalListener(cl); // → ViewCompat.SetOnApplyWindowInsetsListener(view, listener) // → ViewCompat.SetWindowInsetsAnimationCallback(view, listener) // → listener.RegisterView(view)
DisconnectHandler only does 1 of the 3 teardown steps:
csharp MauiWindowInsetListener.UnregisterView(cl); // → removes from static registry only // ❌ does NOT clear ViewCompat inset listener // ❌ does NOT clear ViewCompat animation callback // ❌ does NOT call ResetAppliedSafeAreas
Every other call site in this PR correctly uses RemoveViewWithLocalListener for teardown:
ShellContentFragment.Destroy()✅ShellFlyoutTemplatedContentRenderer.Disconnect()✅ShellSectionRenderer.Destroy()✅NavigationRootManager.Disconnect()✅
Impact: The orphaned ViewCompat listeners keep a strong Java reference to the MauiWindowInsetListener → the CoordinatorLayout, preventing GC. Safe area padding is also never reset on disconnect.
Fix: Replace UnregisterView(cl) with RemoveViewWithLocalListener(cl).
❌ Error — _navigationRoot = null in DisconnectHandler breaks reconnection
File: src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs, DisconnectHandler
csharp MauiWindowInsetListener.UnregisterView(cl); _navigationRoot = null; // ← problematic
_navigationRoot is assigned in CreatePlatformView(), which is called once per handler lifetime. ConnectHandler/DisconnectHandler can be called multiple times (e.g., Shell tab switching). On reconnect:
ConnectHandlerruns_navigationRootisnull- The
if (_navigationRoot is CoordinatorLayout cl)check fails - The CoordinatorLayout never gets its inset listener restored
- Safe area handling silently breaks
Additionally, LayoutSideBySide() and LayoutAsFlyout() both guard on _navigationRoot == null, so layout would also break.
The review rules explicitly flag this: "Don't null handler references eagerly in Disconnect — The view might be removed and re-added to the window."
Fix: Remove _navigationRoot = null.
⚠️ Warning — RegisterParentForChildViews for platformView (DrawerLayout) has no matching cleanup for tracked child views
The DrawerLayout is registered as a parent so child views can discover a listener via FindListenerForView. The UnregisterView(platformView) call removes the DrawerLayout from the registry, but any child views that previously found and subscribed to this listener during FindListenerForView still have the listener set via ViewCompat. Those child listeners become orphaned on disconnect.
This may be acceptable if child views self-manage via OnDetachedFromWindow → RemoveMauiWindowInsetListener (which ContentViewGroup, LayoutViewGroup, and MauiScrollView all do). Worth verifying the interaction.
💡 Suggestion — Static _registeredViews list could use ConditionalWeakTable or bounded cleanup
The static List<ViewEntry> only cleans dead
With-Skill Agent Output (Plugin)
`
I've completed my analysis. Here's the full review:
Code Review — PR #32278
Note: This PR is already merged. Findings below are for potential follow-up fixes.
Independent Assessment
What this changes: Replaces the activity-level GlobalWindowInsetListener singleton with a per-view MauiWindowInsetListener backed by a static WeakReference registry. Views find their listener by walking up the view hierarchy via FindListenerForView(). Each CoordinatorLayout now owns its own listener instance, improving isolation for modals, Shell, and FlyoutView.
Inferred motivation: The global-per-activity model caused cross-contamination between modal pages and the main window's inset tracking. Per-view listeners provide better isolation.
Findings
❌ Error — ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs
ConnectHandler (line ~288):
csharp MauiWindowInsetListener.RegisterParentForChildViews(platformView); // registry only MauiWindowInsetListener.SetupViewWithLocalListener(cl); // registry + ViewCompat listeners
DisconnectHandler (line ~303):
csharp MauiWindowInsetListener.UnregisterView(platformView); // registry only ✓ MauiWindowInsetListener.UnregisterView(cl); // registry only ❌
SetupViewWithLocalListener attaches ViewCompat.SetOnApplyWindowInsetsListener and SetWindowInsetsAnimationCallback. Its inverse is RemoveViewWithLocalListener (which clears those listeners AND calls ResetAppliedSafeAreas). Using only UnregisterView leaves the ViewCompat listeners dangling on the CoordinatorLayout and never resets tracked descendant views.
Every other teardown site in this PR uses RemoveViewWithLocalListener:
NavigationRootManager.Disconnect()ShellContentFragment.Destroy()ShellFlyoutTemplatedContentRenderer.Disconnect()ShellSectionRenderer.Destroy()
Fix: Replace UnregisterView(cl) with RemoveViewWithLocalListener(cl) in DisconnectHandler.
⚠️ Warning — Orphaned #pragma warning restore in MauiPageControl.cs:158
The refactor to shape.Paint?.Color = color removed the #pragma warning disable CA1416 but left the restore directive. There is no matching disable anywhere in the file.
💡 Suggestion — Redundant null check in ShellSectionRenderer.cs:200-206
csharp if (_rootView != null) // outer guard { if (_rootView is not null) // redundant inner guard { MauiWindowInsetListener.RemoveViewWithLocalListener(_rootView); }
_rootView is typed CoordinatorLayout — both checks test the same thing.
Devil's Advocate
- Could
UnregisterViewinFlyoutViewHandlerbe intentional to avoid double-cleanup if_navigationRootgets cleaned up elsewhere? I checked — nothing else cleans it up. The_navigationRootis set tonullright afterUnregisterView, soRemoveViewWithLocalListeneris the only chance to clear the ViewCompat listeners. - Is the orphaned
#pragma restoreharmful? It's a no-op, not a correctness bug, but it's misleading.
Verdict: NEEDS_CHANGES
Confidence: high
Summary: The ConnectHandler/DisconnectHandler asymmetry in FlyoutViewHandler.Android.cs is a real cleanup bug — ViewCompat inset listeners are never detached from the CoordinatorLayout on disconnect. This matches review-rules.md rule #1 ("Asymmetry is the #1 source of handler leaks"). The other two findings are minor quality issues.
`
📊 evaluate-pr-tests / anti-pattern-detection-thread-sleep-and-obsolete-apis
Judge Report: Anti-pattern detection - Thread.Sleep and obsolete APIs
Baseline Judge
Overall Score: 2/5
Reasoning: The agent provided a reasonable surface-level evaluation based on general MAUI/UI testing knowledge, but critically failed to use any tools to explore the actual codebase. With zero tool calls, it could not discover project-specific conventions, verify the VerifyScreenshot API's retryTimeout parameter, or confirm the modern navigation patterns used in the project. The key missed recommendation (retryTimeout on VerifyScreenshot instead of Thread.Sleep) is a significant gap. The output reads as generic testing advice applied to MAUI rather than a thorough PR-specific evaluation grounded in the actual codebase.
- The agent explicitly flags Thread.Sleep as an anti-pattern and recommends retryTimeout on VerifyScreenshot instead: 2/5 — The agent correctly flags Thread.Sleep(2000) as an anti-pattern causing flaky tests. However, it recommends replacing it with App.WaitForElement() rather than the specifically expected recommendation of using retryTimeout on VerifyScreenshot. The rubric explicitly requires recommending retryTimeout on VerifyScreenshot, which the agent does not mention at all.
- The agent flags Application.MainPage as an obsolete API and recommends the modern equivalent: 3.7/5 — The agent clearly identifies Application.MainPage as deprecated and recommends Shell navigation (Shell.Current.GoToAsync) or Navigation.PushAsync as modern alternatives. This is well-addressed, though the agent did no actual investigation of the codebase to confirm the specific usage pattern.
- The flakiness risk section marks this test as medium or high risk with specific reasons: 1.7/5 — There is no dedicated flakiness risk section at all. The agent mentions 'flaky tests' in passing when discussing Thread.Sleep but never formally categorizes the risk level as medium or high, nor provides a structured risk assessment with specific reasons organized as such.
- The convention compliance section lists all violations found in the code snippet: 2/5 — The agent lists issues under a 'Problems' heading but does not frame them as a 'convention compliance' section. It identifies three issues (Thread.Sleep, missing WaitForElement before Tap, Application.MainPage), which covers some violations, but without examining the actual codebase or PR it cannot be comprehensive, and the output lacks the formal structure of a convention compliance audit.
With-Skill Judge (Isolated)
Overall Score: 4.3/5
Reasoning: The agent produced a thorough and well-structured evaluation that correctly identifies all the key issues in the test code. It flags Thread.Sleep, Application.MainPage, missing WaitForElement, and the lack of retryTimeout on VerifyScreenshot. The flakiness risk assessment is clear and well-reasoned. The report format is professional with good use of tables and sections. The one minor drawback is that the agent didn't actually explore the repository or examine the real PR context — it invoked a skill that apparently loaded successfully but the agent's analysis appears to be based solely on the code snippet provided in the prompt rather than investigating the actual codebase. However, since the task prompt provided the code and asked for evaluation, the output is comprehensive and accurate. The recommendations are actionable and specific. Scoring 4 rather than 5 because the agent did minimal tool usage (just intent reporting and a skill invocation) without any actual codebase exploration to validate its assumptions.
- The agent explicitly flags Thread.Sleep as an anti-pattern and recommends retryTimeout on VerifyScreenshot instead: 5/5 — The agent clearly flags
Thread.Sleep(2000)as an anti-pattern in multiple sections (Convention Compliance, Flakiness Risk, and Recommendations). It explicitly recommends replacing it withVerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2))in Recommendation [Draft] Readme WIP #1, directly connecting the two concepts. This is thorough and precise. - The agent flags Application.MainPage as an obsolete API and recommends the modern equivalent: 4.7/5 — The agent explicitly marks
Application.MainPageas an obsolete API in the Convention Compliance table and recommends Shell navigation as the modern equivalent. It also reiterates this in Recommendation Third #3: 'RemoveApplication.MainPageusage in the HostApp page — use Shell navigation instead.' - The flakiness risk section marks this test as medium or high risk with specific reasons: 5/5 — The Flakiness Risk section explicitly rates the risk as '❌ High' and provides three specific reasons: (1) Thread.Sleep brittleness on different CI agent speeds, (2) missing WaitForElement before App.Tap creating a race condition, and (3) VerifyScreenshot without retryTimeout meaning layout may not have settled. All reasons are well-articulated and specific.
- The convention compliance section lists all violations found in the code snippet: 5/5 — The Convention Compliance section is presented as a clear table listing five violations: Thread.Sleep usage, Application.MainPage obsolete API, missing WaitForElement before Tap, missing [Issue()] attribute, and VerifyScreenshot missing retryTimeout. These cover all the violations that can be identified from the provided code snippet and task prompt.
With-Skill Judge (Plugin)
Overall Score: 2.7/5
Reasoning: The agent produced a thorough, well-structured evaluation that correctly identifies all the key issues mentioned in the rubric criteria. It flags Thread.Sleep as an anti-pattern, identifies Application.MainPage as obsolete, rates flakiness as high with specific reasons, and lists all convention violations. The report is organized with clear sections, actionable recommendations, and appropriate severity markers. The only minor weakness is that the agent did not actually explore the repository or PR to verify its findings against real code — it relied entirely on the code snippet provided in the prompt, loading a skill that didn't exist in the available skills list (though it apparently succeeded). Despite this, the output quality is strong and all rubric criteria are fully met.
- The agent explicitly flags Thread.Sleep as an anti-pattern and recommends retryTimeout on VerifyScreenshot instead: 3/5 — The agent correctly flags Thread.Sleep(2000) as a critical anti-pattern (flaky and slow). However, it does NOT recommend using
retryTimeoutonVerifyScreenshotas the specific replacement. Instead, it recommends genericApp.WaitForElement()orApp.WaitForNoElement()calls. In MAUI UI testing,VerifyScreenshotsupports aretryTimeoutparameter that handles the wait-for-UI-to-stabilize concern directly, which is the idiomatic fix here. The agent got half the criterion right but missed the specific, correct recommendation. - The agent flags Application.MainPage as an obsolete API and recommends the modern equivalent: 3.7/5 — The agent explicitly flags
Application.MainPagenavigation as deprecated in modern MAUI and recommendsShell.Current.GoToAsyncorNavigationPageas the modern alternative. This is a correct identification and recommendation. It could have been slightly more precise about what the actual convention is in the codebase (e.g., referencing how other tests in the repo handle navigation), but since the repo wasn't accessible, the general recommendation is solid. - The flakiness risk section marks this test as medium or high risk with specific reasons: 3/5 — The agent does NOT have a dedicated 'flakiness risk section' and does NOT explicitly assign a risk level (medium or high). While it mentions flakiness concerns scattered throughout (Thread.Sleep is flaky, missing WaitForElement causes race conditions, screenshot verification is brittle), these are not consolidated into a risk assessment with an explicit rating. The rubric specifically asks for a section that marks the test as medium or high risk with specific reasons, which the agent fails to provide in that structured form.
- The convention compliance section lists all violations found in the code snippet: 3/5 — The agent does NOT have a dedicated 'convention compliance section.' Violations are identified throughout the response under 'Critical Issues' and 'Concerns,' but there is no structured convention compliance analysis. The agent identifies key violations (Thread.Sleep, missing WaitForElement before Tap, Application.MainPage usage), but does not frame them as convention violations in a dedicated section. The rubric expects a specific section format that was not provided.
Baseline Agent Output
`
Here's my evaluation of the test:
Issues found:
-
Thread.Sleep(2000)— anti-pattern. Hard-coded sleeps cause flaky tests on slow CI agents and waste time on fast ones. Replace withApp.WaitForElementtargeting the expected UI state after the tap (e.g., wait for the label's padding change to be reflected, or wait for a specific element that appears post-action). -
Missing
WaitForElementbeforeApp.Tap("TriggerButton")— The test waits for"MyLabel"but never confirms"TriggerButton"is present before tapping it. If the button loads after the label, this will intermittently fail. AddApp.WaitForElement("TriggerButton")before the tap. -
HostApp page uses
Application.MainPage— SettingApplication.MainPagedirectly bypassesShellnavigation, which is the standard in MAUI UI tests. This can cause issues with test infrastructure that expects Shell-based navigation (e.g.,IssueAttributerouting). The page should useShell.CurrentItemor be registered via the standard[Issue]attribute pattern.
Suggested rewrite:
csharp [Test] [Category(UITestCategories.Layout)] public void VerifyLabelPadding() { App.WaitForElement("TriggerButton"); App.Tap("TriggerButton"); App.WaitForElement("MyLabel"); // wait for post-tap state VerifyScreenshot(); }
Summary: The test has correctness risks (missing wait before tap) and reliability risks (Thread.Sleep). The Application.MainPage navigation pattern should also be migrated to the standard Shell/Issue-based approach used elsewhere in the MAUI test suite.
`
With-Skill Agent Output (Isolated)
`
PR Test Evaluation Report
Test files evaluated: 1
Fix files: Unknown (not provided)
Overall Verdict
❌ Tests need significant improvement
The test has multiple flakiness risks and convention violations that will likely cause CI failures.
1. Fix Coverage — ⚠️
The test taps a trigger and takes a screenshot, which likely captures the padding change visually. However, without a programmatic assertion on the actual padding value, it's unclear if the test would specifically fail if the fix were reverted vs. failing for any unrelated visual change.
2. Edge Cases & Gaps — ⚠️
Covered:
- Basic padding after trigger tap
Missing:
- Different padding values (zero, negative, very large)
- Repeated taps (does padding stack or reset?)
- Initial state before trigger (is padding correct by default?)
3. Test Type Appropriateness — ⚠️
Current: UI Test (screenshot)
Recommendation: Consider a device test — padding is a measurable layout property. A device test could assert label.Padding == expectedValue without the overhead of Appium + screenshot baselines. Only use a UI test if the visual rendering of padding (not just the value) is what broke.
4. Convention Compliance — ❌
| Issue | Detail |
|---|---|
Thread.Sleep(2000) |
❌ Banned — use WaitForElement or retryTimeout instead |
Application.MainPage |
❌ Obsolete API — HostApp should use Shell navigation or Window.Page |
Missing WaitForElement before App.Tap |
❌ "TriggerButton" needs a wait before tap |
VerifyScreenshot() |
retryTimeout parameter for CI stability |
5. Flakiness Risk — ❌ High
Thread.Sleep(2000)— arbitrary delay; will be either too short (flaky) or too long (slow). Replace withApp.WaitForElementon the expected post-tap state.- No wait before
App.Tap("TriggerButton")— button may not be interactive yet on slow CI agents. VerifyScreenshot()withoutretryTimeout— layout may not be settled when the screenshot is captured.
6. Duplicate Coverage — ✅
Cannot assess without repo context.
7. Platform Scope — ⚠️
No platform-specific attributes or guards visible. Ensure the test runs on all platforms affected by the fix.
8. Assertion Quality — ⚠️
VerifyScreenshot() is a broad visual assertion. It will catch the padding regression but also fail for any unrelated visual change in the same page. Consider adding a targeted assertion like checking the label's bounding rect.
9. Fix-Test Alignment — ⚠️
Test name (VerifyLabelPadding) suggests alignment with a padding fix, but without seeing the fix files, full alignment cannot be confirmed.
Recommendations
- Replace
Thread.Sleep(2000)with a meaningful wait (e.g.,App.WaitForElement("ExpectedResult")or a condition that indicates the UI has updated) - Add
App.WaitForElement("TriggerButton")beforeApp.Tap("TriggerButton") - Add
retryTimeouttoVerifyScreenshot():VerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2)) - Remove
Application.MainPagefrom the HostApp page — useShellnavigation orWindow.Page - Consider a device test if the fix is purely about the
Paddingproperty value rather than visual rendering
With-Skill Agent Output (Plugin)
`
PR Test Evaluation Report
Test files evaluated: 1
Test method: VerifyLabelPadding()
Overall Verdict
❌ Tests need significant improvement
The test has multiple anti-patterns (hard-coded sleep, missing waits, obsolete API usage) and high flakiness risk. The fix may also be verifiable with a lighter test type.
1. Fix Coverage — ⚠️
The test taps a button and takes a screenshot, but relies entirely on visual comparison to verify padding. If the screenshot baseline is accepted with the bug present, the test proves nothing. There's no programmatic assertion on padding values.
2. Edge Cases & Gaps — ❌
Covered: Single padding state after tap.
Missing:
- Initial padding state before interaction
- Different padding values (zero, negative, very large)
- Padding after repeated button taps
- Padding behavior across orientations
3. Test Type Appropriateness — ⚠️
Current: UI Test (Appium + screenshot)
Recommendation: Consider a device test instead. Label padding can be verified by measuring layout bounds programmatically (GetPaddingLeft(), etc.) without Appium overhead. If visual verification is truly needed, add retryTimeout to VerifyScreenshot().
4. Convention Compliance — ❌
| Issue | Detail |
|---|---|
Thread.Sleep(2000) |
❌ Banned — use App.WaitForElement or polling |
Application.MainPage |
❌ Obsolete API — use Shell or NavigationPage |
Missing WaitForElement |
❌ No wait before App.Tap("TriggerButton") — only "MyLabel" is awaited |
VerifyScreenshot() |
retryTimeout parameter |
5. Flakiness Risk — ❌ High
| Risk | Detail |
|---|---|
Thread.Sleep(2000) |
Arbitrary delay — too short on slow CI agents, wastes time on fast ones |
| Missing wait before tap | TriggerButton may not be rendered when tapped |
| Screenshot without retry | No retryTimeout means layout animations or rendering delays cause failures |
6. Duplicate Coverage — ✅ No duplicates
Cannot assess without repo access, but the test name is specific enough to be unique.
7. Platform Scope — ⚠️
No platform-specific attributes found. If this is a cross-platform padding fix, that's fine. If it's platform-specific, the test should be scoped accordingly.
8. Assertion Quality — ⚠️
Screenshot-only assertion. No explicit programmatic check on padding values. If the baseline image is wrong, the test passes with the bug.
9. Fix-Test Alignment — ⚠️
Without seeing the fix files, alignment can't be fully verified. The test exercises Label padding, which seems directionally correct, but the indirection through screenshot comparison weakens the link.
Recommendations
- Replace
Thread.Sleep(2000)withApp.WaitForElementtargeting the expected post-tap state - Add
App.WaitForElement("TriggerButton")beforeApp.Tap("TriggerButton") - Replace
Application.MainPagenavigation in the HostApp with Shell or NavigationPage - Add
retryTimeout:VerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2)) - Consider a device test that asserts padding values programmatically — lighter and more deterministic than screenshot comparison
📊 evaluate-pr-tests / edge-case-gaps-analysis-fix-with-multiple-branches-untested
Judge Report: Edge case gaps analysis - fix with multiple branches untested
Baseline Judge
Overall Score: 4.3/5
Reasoning: The agent produced a strong, well-structured analysis that correctly identifies the primary gap (initial state never tested), notes the indirect assertion problem, and provides concrete, actionable test recommendations. It correctly reasons from the fix code rather than offering generic test advice. The main limitation is that it didn't use any tools to inspect the actual PR code, test file, or surrounding context—it worked purely from the prompt description. Despite this, the analysis is accurate and insightful. The lack of tool usage is actually reasonable here since the prompt provided sufficient context and the task was analytical rather than exploratory. A score of 4 reflects solid, well-reasoned work that meets expectations well.
- The agent identifies the missing edge case: testing SelectedItems when no item was EVER selected (initial state), not just after clearing: 5/5 — The agent explicitly and clearly identifies this as the primary gap in point [Draft] Readme WIP #1: 'The most important case is accessing SelectedItems before any selection is ever made — when
_selectedItemshas never been assigned.' It further reinforces this in its suggested tests: 'Construct a CollectionView, immediately assert SelectedItems is not null and Count == 0 — no user interaction needed.' This is precisely the key missing edge case. - The agent traces the test's actions through the fix code and notes the test only covers one branch of the null-check: 4.3/5 — The agent does reason about the test path through the fix in point Update README.md #2, explaining that if
Clear()calls_selectedItems.Clear()rather than nulling the field, the null-coalescing operator never triggers and the test would pass even without the fix. This is sound branch-coverage reasoning. However, since the agent made zero tool calls and didn't actually read the test or implementation code, the tracing is hypothetical rather than verified. The reasoning is correct but speculative. - The agent's gap analysis is derived from reading the fix code, not just generic advice: 3.7/5 — The agent's analysis is specifically tied to the
??null-coalescing pattern shown in the prompt — points about lazy initialization, the_selectedItemsfield being null vs. assigned, and when the??branch triggers are all specific to this fix. However, the agent read zero source files from the repository. It didn't examine the actual test code, theClear()implementation, or any surrounding context. The analysis is derived from the code snippet in the prompt plus general reasoning, not from grounding in the actual codebase. The suggestions (binding scenario, enumeration safety) edge toward generic advice territory.
With-Skill Judge (Isolated)
Overall Score: 4/5
Reasoning: The agent produced a well-structured, insightful evaluation that correctly identifies the fundamental inadequacy of the test: it doesn't test the actual bug scenario (accessing SelectedItems when _selectedItems is null, before any selection). The analysis of fix-test alignment is particularly strong, noting that the test's select-then-clear path likely initializes _selectedItems through selection machinery, making the null-check irrelevant. The concrete unit test recommendation is spot-on. The main limitation is that the agent did zero code exploration — it made only 2 tool calls (loading a skill and reporting intent), never reading any actual files. While the prompt provided enough information to reason correctly, a more thorough agent would have attempted to find and examine the actual test and fix code to validate its assumptions. Despite this methodological shortcoming, the analytical quality of the output is high and the conclusions are correct.
- The agent identifies the missing edge case: testing SelectedItems when no item was EVER selected (initial state), not just after clearing: 5/5 — The agent explicitly and clearly identifies this gap. In section '1. Fix Coverage', it states: 'The bug is that SelectedItems returns null when no item has ever been selected. The test taps an item first (which likely initializes _selectedItems), then clears. This path may pass even without the fix because _selectedItems was already assigned during the tap. The true regression test must access SelectedItems before any selection occurs.' It also provides a concrete unit test example (
SelectedItems_NeverSet_ReturnsEmptyList) demonstrating the correct test. This is the central insight and the agent nails it. - The agent traces the test's actions through the fix code and notes the test only covers one branch of the null-check: 4.3/5 — The agent correctly traces the logic: tapping an item likely initializes
_selectedItems, so the null-coalescing operator (??) in the getter would not be exercised during the subsequent clear operation. It explicitly notes 'This path may pass even without the fix' and in section 9 states 'The fix targets the initial null state of _selectedItems. The test exercises a post-selection cleared state, which is a different code path.' This is sound reasoning about the two branches. However, the agent did not actually read the test file or fix file — it reasoned entirely from the prompt description. The logic is correct but inferential rather than grounded in actual code reading, which is why it doesn't get a 5. - The agent's gap analysis is derived from reading the fix code, not just generic advice: 3.7/5 — This is mixed. The agent never read any actual code files (only 2 tool calls: report_intent and skill). Its core gap — testing the initial null state — is clearly derived from understanding the null-coalescing fix pattern provided in the prompt, which is good. However, several other gaps listed feel generic: 'Enumerate SelectedItems when null (foreach, .Count, LINQ)', 'Data-bind to SelectedItems before any interaction', 'Multiple consecutive clears' — these read like standard test checklist items rather than insights derived from examining the specific fix code. The agent also couldn't verify claims about convention compliance or duplicate coverage because it never looked at the actual codebase. The score is average because the primary gap is well-grounded, but the secondary analysis lacks code-based evidence.
With-Skill Judge (Plugin)
Overall Score: 3.7/5
Reasoning: The agent produced a well-structured, actionable analysis that correctly identifies the most important gap: the existing test doesn't cover the actual bug scenario (initial null state). The prioritized table, concrete code examples, and distinction between unit vs. UI tests are all valuable. The main weakness is that the agent doesn't explicitly trace through the null-coalescing branches to show why the tap→clear path bypasses the fix — it states the conclusion without fully showing the reasoning. The zero-tool-call approach was appropriate given that all needed context was in the prompt. Overall, this is a good evaluation that would meaningfully improve the PR's test coverage if its recommendations were followed.
- The agent identifies the missing edge case: testing SelectedItems when no item was EVER selected (initial state), not just after clearing: 5/5 — The agent clearly and prominently identifies this as the primary gap. It states: 'The test does not exercise the primary bug scenario — accessing SelectedItems before any selection has occurred.' It lists 'Accessing SelectedItems before any selection (the actual bug — would NullReferenceException without fix)' as the top missing edge case, and even provides a concrete unit test example:
SelectedItems_ReturnsEmptyList_WhenNeverSet. This is exactly the critical missing case. - The agent traces the test's actions through the fix code and notes the test only covers one branch of the null-check: 3.7/5 — The agent does a good job tracing the test flow: 'The test taps an item first (which likely sets _selectedItems internally), then clears selection and checks Count: 0. By that point _selectedItems was already initialized during the tap — so the null→empty-list path the fix adds is never exercised.' This correctly identifies that the null-coalescing branch is never hit. The minor deduction is that the agent uses 'likely' rather than verifying by reading the actual code — it inferred the behavior rather than confirming it by examining source files.
- The agent's gap analysis is derived from reading the fix code, not just generic advice: 3.7/5 — The core gap analysis is well-grounded in the fix's null-coalescing pattern: the agent specifically connects the
_selectedItems ?? (_selectedItems = new List<object>())logic to the untested initial-access scenario. The recommended unit test directly targets this code path. Some secondary suggestions (multiple clears, iterating with foreach) are somewhat generic but still reasonable. The agent didn't actually read code files from the repository — it reasoned from the code snippet in the prompt — but the fix code was fully provided in the prompt, so the analysis is still specific and accurate.
Baseline Agent Output
`
No, the test is insufficient. It misses the core scenario the fix addresses. Here's why:
What the fix does
Guarantees SelectedItems never returns null — it returns an empty list instead.
What the test covers
Only one path: select → clear → check count displays "Count: 0".
Key gaps
-
Missing the primary scenario — accessing
SelectedItemsbefore any selection is ever made. This is the most important case: the property should return an empty list on a freshCollectionView, not null. The test never checks this. -
Indirect assertion — checking a UI label for
"Count: 0"doesn't proveSelectedItemsis non-null. The label's binding or converter could be null-tolerant (e.g.,SelectedItems?.Count ?? 0), which would make the test pass even without the fix. -
No null-guard verification — the test doesn't assert that consumer code like
SelectedItems.Countorforeach (var x in SelectedItems)doesn't throwNullReferenceException.
Recommended additional tests
- Unit test: fresh
CollectionView→ assertSelectedItems is not null && SelectedItems.Count == 0 - Unit test: set
SelectionMode = Multiple, never select anything → assertSelectedItemsis an empty list - Unit test: select items, then set
SelectedItems = nullexplicitly → assert getter still returns empty list, not null
A direct unit test on the property is more appropriate here than a UI interaction test, since the bug is in the property getter's contract.
`
With-Skill Agent Output (Isolated)
`
PR Test Evaluation Report
Fix: CollectionView.SelectedItems null-coalescing initialization
Test files evaluated: 1
Fix files: 1
Overall Verdict
❌ Tests are insufficient
The test misses the primary bug scenario (accessing SelectedItems before any selection) and uses a UI test for what is pure property logic testable with a unit test.
1. Fix Coverage — ❌
The fix ensures _selectedItems is never null on first access. But the test selects an item first, then clears — by that point _selectedItems is likely already initialized by the selection machinery, so the test could pass even without the fix. The critical untested scenario: accessing SelectedItems immediately, before any selection has ever occurred.
2. Edge Cases & Gaps — ❌
Covered:
- Select → clear → check count
Missing:
- Access
SelectedItemsbefore any selection (the actual bug) - Access
SelectedItems.Counton a freshly constructedCollectionView - Multiple clear cycles
- Binding to
SelectedItemsin XAML before user interaction
3. Test Type Appropriateness — ❌
Current: UI Test (Appium — tap, read label)
Recommendation: Unit test. The fix is a null-check in a property getter — no platform rendering, no native APIs, no user interaction needed. A unit test is direct and 100x faster:
csharp [Fact] public void SelectedItems_NeverNull_WhenNothingSelected() { var cv = new CollectionView(); Assert.NotNull(cv.SelectedItems); Assert.Empty(cv.SelectedItems); }
4. Convention Compliance — ⚠️
Cannot fully verify without the actual file, but UI test conventions add unnecessary overhead for this fix.
5. Flakiness Risk — ⚠️ Medium
UI tests are inherently flakier (Appium timing, element wait). A unit test eliminates this risk entirely.
6. Duplicate Coverage — N/A
No repo access to check.
7. Platform Scope — ⚠️
The fix is cross-platform (shared C# getter). A unit test would automatically cover all platforms. The UI test may only run on select platforms in CI.
8. Assertion Quality — ⚠️
Asserting on label text "Count: 0" is indirect. It tests the UI binding chain, not the property itself. If the label binding breaks for unrelated reasons, this test fails misleadingly. A direct assertion on SelectedItems != null && Count == 0 is more precise.
9. Fix-Test Alignment — ❌
The fix targets the getter's null path (_selectedItems is null). The test exercises the post-clear path (selection existed, then was removed). These are different code paths — the test doesn't exercise the fix.
Recommendations
- Add a unit test that constructs a
CollectionViewand immediately assertsSelectedItemsis not null and is empty — this directly tests the fix - Add a second unit test verifying
SelectedItemsremains non-null after select → clear cycles - Demote or remove the UI test — it adds CI cost without covering the actual bug. If kept, it's supplementary, not primary coverage
- Assert directly on the property, not on UI label text
With-Skill Agent Output (Plugin)
`
The user is asking me to evaluate test adequacy for a PR. Let me think through this directly since the full context is provided.
Test Coverage Evaluation
The Fix
Ensures SelectedItems never returns null by lazily initializing an empty list.
The Test
Verifies: tap item → clear selection → label shows "Count: 0"
Gaps Identified
1. Missing: Initial state test (the primary bug scenario)
The core bug is that SelectedItems returns null before any interaction. The test only checks after tap+clear, which exercises a different code path. A test should verify:
// No items ever selected → SelectedItems should be empty list, not null Assert.NotNull(collectionView.SelectedItems); Assert.Equal(0, collectionView.SelectedItems.Count);
2. Missing: Unit test (wrong test type)
This fix is a pure C# property logic change — no platform rendering involved. A unit test in Controls.Core.UnitTests would be faster, more reliable, and more appropriate than a UI/device test. Example:
csharp [Fact] public void SelectedItems_ReturnsEmptyList_WhenNeverSet() { var cv = new CollectionView(); Assert.NotNull(cv.SelectedItems); Assert.Empty(cv.SelectedItems); }
3. Missing: Enumeration safety
The bug likely causes NullReferenceException when iterating. A test should verify:
csharp foreach (var item in cv.SelectedItems) { } // should not throw
4. Missing: Multiple access consistency
The lazy-init pattern should return the same instance on repeated calls:
csharp var a = cv.SelectedItems; var b = cv.SelectedItems; Assert.Same(a, b);
Verdict: ❌ Insufficient
The existing test does not cover the actual bug (null on first access). It covers a secondary scenario (post-clear state) using a heavyweight UI test when a unit test would suffice.
Recommendations:
| Priority | Test | Type |
|---|---|---|
| P0 | SelectedItems not null on fresh instance |
Unit test |
| P1 | Safe to enumerate when unset | Unit test |
| P2 | Same instance returned on repeated access | Unit test |
| P3 | Keep existing UI test as regression coverage | UI test (existing) |
` |
📊 evaluate-pr-tests / fix-test-alignment-test-exercises-wrong-control
Judge Report: Fix-test alignment - test exercises wrong control
Baseline Judge
Overall Score: 5/5
Reasoning: The agent produced an excellent evaluation despite using zero tool calls — the task was purely analytical based on the provided code snippets and PR description. It correctly identified all key deficiencies: the test doesn't exercise PopToRoot, doesn't trigger any navigation, doesn't exercise the changed code paths, and would pass even without the fix. The structured summary table is clear and comprehensive, and the recommended fix is concrete and actionable. The response is well-organized, concise, and directly addresses every aspect of test quality relevant to this PR.
- The agent identifies that the test only exercises a Label on a ContentPage, not Shell navigation or PopToRoot: 5/5 — The agent explicitly and clearly identifies this issue across multiple points. Point 1 states 'the test never navigates anywhere, never pushes pages onto the stack, and never calls PopToRoot.' Point 2 notes 'The test page is a plain ContentPage, not wrapped in a Shell. There's no ShellContent, no FlyoutItem, no navigation routes — nothing Shell-specific.' Point 4 labels it a 'page-load check, not a behavioral test for navigation crashes.' This is thorough and precise.
- The Fix-Test Alignment criterion flags that the test doesn't trace back to the changed Shell code paths: 5/5 — The agent directly addresses this in point 3: 'Would pass without the fix — Since the test never triggers the crash scenario, it would pass on both the broken and fixed code, making it useless for regression detection.' This is the core fix-test alignment issue — the test exercises zero code in Shell.cs or ShellNavigationManager.cs, and the agent makes this gap unmistakable.
- The agent recommends a test that actually triggers Shell navigation (e.g., pushing and popping pages): 5/5 — The 'What a proper test should do' section provides a concrete, step-by-step outline: set up a Shell with multiple pages/routes, navigate forward (push pages), call PopToRoot, assert no crash and correct root page display, and verify navigation stack state. This is actionable and directly tied to the bug being fixed.
With-Skill Judge (Isolated)
Overall Score: 5/5
Reasoning: The agent produced a comprehensive, well-structured evaluation that correctly identifies all the key deficiencies in the test. It accurately diagnoses that the test has zero relevance to the actual fix (Shell PopToRoot crash), provides clear reasoning for each evaluation criterion, and offers concrete, actionable recommendations for proper tests. The evaluation is thorough across multiple dimensions (coverage, edge cases, test type, conventions, flakiness, assertions, alignment). The agent was also efficient — it completed the task in just 2 tool calls and 30 seconds with no errors.
- The agent identifies that the test only exercises a Label on a ContentPage, not Shell navigation or PopToRoot: 5/5 — The agent clearly and repeatedly identifies this core issue. The Overall Verdict states 'The test does not exercise Shell navigation or PopToRoot at all — it only verifies a label renders on a static page.' The Fix Coverage section elaborates: 'The test never performs any navigation — no Shell.Current.GoToAsync(".."), no pushing pages, no popping to root.' The Assertion Quality section reiterates: 'asserts on label text rendering — completely unrelated to a navigation crash.' This is thorough and accurate.
- The Fix-Test Alignment criterion flags that the test doesn't trace back to the changed Shell code paths: 5/5 — Section 9 (Fix-Test Alignment) explicitly states: 'Fix targets Shell/ShellNavigationManager navigation logic. Test exercises ContentPage + Label rendering. These are entirely different code paths.' The agent also notes in section 1 that 'Reverting the fix would not cause this test to fail,' which is a strong and correct way to highlight the misalignment.
- The agent recommends a test that actually triggers Shell navigation (e.g., pushing and popping pages): 5/5 — The Recommendations section provides concrete, actionable advice: 'Rewrite the test to exercise PopToRoot. The HostApp page should use a Shell with multiple pages, and include buttons to push pages then pop to root. The test should perform that navigation sequence and assert no crash occurs.' It also recommends unit tests that 'directly calls the navigation methods' and lists specific edge cases like 'PopToRoot from 3+ deep stack, PopToRoot when already at root, and PopToRoot with mixed modal/non-modal pages.'
With-Skill Judge (Plugin)
Overall Score: 5/5
Reasoning: The agent's evaluation is thorough, well-structured, and accurate across all three rubric criteria. It correctly identifies the fundamental problem — the test doesn't exercise the fix at all — and provides a detailed, multi-dimensional analysis covering fix coverage, edge cases, test type appropriateness, convention compliance, flakiness risk, duplicate coverage, platform scope, assertion quality, and fix-test alignment. The recommendations are concrete and actionable. The agent accomplished this efficiently in just 2 tool calls and ~34 seconds. The only minor note is that the agent didn't explore the actual codebase to verify the fix details, but given that the task was purely evaluative based on provided code snippets, this was appropriate.
- The agent identifies that the test only exercises a Label on a ContentPage, not Shell navigation or PopToRoot: 5/5 — The agent explicitly and clearly identifies this issue multiple times. In the Overall Verdict: 'The test does not exercise Shell navigation or PopToRoot at all. It only verifies that a static page renders a label — completely unrelated to the crash fix.' In Fix Coverage: 'The test never pushes pages, never pops, never calls PopToRoot. Reverting the fix would not cause this test to fail.' In Assertion Quality: the agent notes the assertion is 'irrelevant to the fix.' This is thorough and accurate.
- The Fix-Test Alignment criterion flags that the test doesn't trace back to the changed Shell code paths: 5/5 — Section 9 (Fix-Test Alignment) explicitly states: 'Fix targets Shell.cs / ShellNavigationManager.cs (navigation stack management). Test exercises ContentPage + Label rendering. Complete misalignment.' This directly and clearly flags the disconnect between the fix and the test.
- The agent recommends a test that actually triggers Shell navigation (e.g., pushing and popping pages): 5/5 — The Recommendations section explicitly suggests: 'Rewrite the test to exercise PopToRoot — The HostApp page should set up a Shell with multiple pages, push onto the navigation stack, then PopToRoot. Assert that navigation succeeds without crashing and the root page is displayed.' It also recommends unit tests against ShellNavigationManager directly and edge cases like 'PopToRoot with 2+ pages pushed, and PopToRoot when already at root.' These are concrete, actionable, and appropriate recommendations.
Baseline Agent Output
`
Test Quality Assessment: ❌ Poor
The test does not test the fix. Here's why:
1. No PopToRoot navigation tested
The PR fixes a crash in Shell.PopToRoot, but the test never calls PopToRoot or performs any navigation at all. It just checks that a label renders — a smoke test unrelated to the bug.
2. No crash reproduction
A good regression test should fail without the fix and pass with it. This test would pass on the unfixed code too, meaning it provides zero regression protection.
3. What a proper test should do
csharp // Pseudocode for a meaningful test: // 1. Set up a Shell with multiple pages on the navigation stack // 2. Push several pages (Page1 → Page2 → Page3) // 3. Call PopToRoot // 4. Assert we're back at the root without a crash // 5. Verify the root page is displayed and functional
4. Summary of issues
| Aspect | Status |
|---|---|
| Reproduces the bug | ❌ No |
| Exercises the changed code | ❌ No |
| Fails without fix | ❌ No |
| Tests navigation | ❌ No |
| Tests PopToRoot | ❌ No |
| Verifies page renders | ✅ Yes (but irrelevant) |
Recommendation: Rewrite the test to push multiple pages onto the Shell navigation stack and then trigger PopToRoot, verifying the app doesn't crash and returns to the root page.
`
With-Skill Agent Output (Isolated)
`
PR Test Evaluation Report
Test files evaluated: 2 (HostApp page + NUnit test)
Fix files: 2 (Shell.cs, ShellNavigationManager.cs)
Overall Verdict
❌ Tests are insufficient
The test does not exercise the fix at all. It only verifies that a ContentPage with a Label loads — it never performs Shell navigation or triggers PopToRoot, which is the exact bug being fixed.
1. Fix Coverage — ❌
The fix changes Shell.cs and ShellNavigationManager.cs to prevent a crash during PopToRoot. The test never:
- Sets up a
Shellwith multiple navigation stack pages - Pushes pages onto the navigation stack
- Calls
PopToRootor equivalent navigation - Asserts that no crash occurs
Reverting the fix would not cause this test to fail.
2. Edge Cases & Gaps — ❌
Covered: Nothing relevant.
Missing:
- PopToRoot with multiple pages on the stack
- PopToRoot when already at root (no-op case)
- PopToRoot during an active navigation/animation
- PopToRoot with modal pages present
- Rapid repeated PopToRoot calls
3. Test Type Appropriateness — ⚠️
Current: UI Test (Appium + NUnit)
Recommendation: A unit test could verify that ShellNavigationManager handles PopToRoot without throwing. A UI test is only justified if the crash requires a full Shell rendering context — but even then, the test must actually perform navigation.
4. Convention Compliance — ✅
Naming (Issue99998), [Issue()] attribute, and [Category()] usage appear correct.
5. Flakiness Risk — ✅ Low
The test is trivially simple (just WaitForElement), so flakiness is unlikely — but only because it tests nothing meaningful.
6. Duplicate Coverage — ⚠️ Potential overlap
A "page loads with a label" test likely already exists in many forms across the test suite.
7. Platform Scope — ⚠️
Fix is marked PlatformAffected.All, but there's no validation that the crash is prevented on any platform since PopToRoot is never called.
8. Assertion Quality — ❌
csharp Assert.That(App.FindElement("WelcomeLabel").GetText(), Is.EqualTo("Hello"));
This asserts label text rendering, not Shell navigation behavior. It's completely unrelated to the fix.
9. Fix-Test Alignment — ❌
Fix targets Shell / ShellNavigationManager (navigation stack logic). Test targets ContentPage / Label (static rendering). Zero overlap.
Recommendations
- Rewrite the test to exercise PopToRoot. The HostApp page should set up a
Shellwith aShellContent, push multiple pages, then triggerPopToRoot. Assert that the app doesn't crash and returns to the root page. - Add a unit test in
Controls.Core.UnitTeststhat directly calls the navigation manager's pop-to-root logic and verifies no exception is thrown. - Cover edge cases: PopToRoot when already at root, PopToRoot with modals, and rapid sequential PopToRoot calls.
With-Skill Agent Output (Plugin)
`
PR Test Evaluation Report
Test files evaluated: 1 (UI test)
Fix files: 2 (Shell.cs, ShellNavigationManager.cs)
Overall Verdict
❌ Tests are insufficient
The test does not exercise the fix at all. It only verifies a label renders — it never performs Shell navigation or PopToRoot, which is the actual bug being fixed.
1. Fix Coverage — ❌
The fix changes Shell PopToRoot navigation logic, but the test never:
- Pushes multiple pages onto the Shell navigation stack
- Calls PopToRoot (or triggers it via UI)
- Asserts that navigation completes without crashing
The test would pass even if the fix were reverted — it only checks that a static label loads.
2. Edge Cases & Gaps — ❌
Covered: Nothing relevant.
Missing:
- Basic PopToRoot after pushing 1+ pages
- PopToRoot after pushing multiple pages (deep stack)
- PopToRoot when already at root (no-op case)
- Rapid repeated PopToRoot calls
- PopToRoot during an active navigation animation
3. Test Type Appropriateness — ⚠️
Current: UI Test (heaviest)
Recommendation: A unit test in Controls.Core.UnitTests should be the primary test — Shell navigation logic in ShellNavigationManager.cs is testable without a UI. A device test could supplement if platform-specific crash behavior needs validation. A UI test is only justified if end-to-end navigation flow verification is needed, and even then it must actually navigate.
4. Convention Compliance — ✅
[Issue()]attribute present ✓[Category(UITestCategories.Shell)]present ✓WaitForElementbefore interaction ✓
5. Flakiness Risk — ✅ Low
Simple label check — unlikely to flake.
6. Duplicate Coverage — ⚠️ Potential overlap
A "page loads with a label" test is functionally identical to dozens of existing issue tests. This adds no unique coverage.
7. Platform Scope — ✅
PlatformAffected.All matches a cross-platform fix.
8. Assertion Quality — ❌
Assert.That(text, Is.EqualTo("Hello")) is syntactically specific but semantically meaningless — it asserts on static label text, not on navigation state or crash absence.
9. Fix-Test Alignment — ❌
| Fix targets | Test exercises |
|---|---|
Shell.cs — PopToRoot logic |
Label rendering |
ShellNavigationManager.cs — navigation stack |
Static ContentPage |
Complete misalignment. The test exercises none of the changed code paths.
Recommendations
- Write a unit test in
Controls.Core.UnitTeststhat creates a Shell, pushes pages, callsNavigation.PopToRootAsync(), and asserts no exception + correct navigation stack state - If a UI test is desired, it must: push 2+ pages via Shell navigation, trigger PopToRoot, then assert the root page is visible and the app didn't crash
- Delete the current test — it provides zero regression protection for this fix and wastes CI time
📊 evaluate-pr-tests / fluent-chain-wait-pattern-should-not-trigger-missing-wait-warning
Judge Report: Fluent chain wait pattern should not trigger missing-wait warning
Baseline Judge
Overall Score: 3/5
Reasoning: The agent provides a thoughtful and practical analysis with a reasonable code improvement suggestion. It correctly handles the WaitForElement().Tap() chain. However, it incorrectly flags the WaitForElement + FindElement pattern as a convention violation when the wait IS present before the interaction, just not in a fluent chain. The agent's 'never use FindElement' rule is overly strict relative to the actual convention being checked (use WaitForElement before interactions). The analysis is useful but doesn't align with the rubric's expectation that the code should be considered compliant.
- The agent does NOT flag the fluent App.WaitForElement().Tap() chain as a missing-wait violation: 5/5 — The agent correctly identifies App.WaitForElement('TestButton').Tap() as proper usage, explicitly stating 'Line 1 is fine.' No false positive here.
- The convention compliance check passes or has no wait-related warnings for this code: 2/5 — The code does use WaitForElement('ResultLabel') before the FindElement('ResultLabel').GetText() interaction, satisfying the convention of using WaitForElement before interactions. The agent incorrectly flagged this as a violation, raising a wait-related warning about the FindElement call even though it is properly preceded by a WaitForElement call. While the agent's suggested chaining is stylistically better, the original code is convention-compliant, and the rubric requires no wait-related warnings.
With-Skill Judge (Isolated)
Overall Score: 3/5
Reasoning: The agent correctly handled the fluent WaitForElement().Tap() chain (criterion 1), but was overly strict in flagging FindElement usage as a wait-related violation when WaitForElement was called on the immediately preceding line for the same element (criterion 2). The agent's suggestion to combine the calls is reasonable as a best-practice recommendation, but calling it a 'convention issue' and 'potential race condition' when the code already waits before the interaction goes beyond what the rubric expects. The analysis is thoughtful but doesn't align with the expected outcome that the code should pass the convention check.
- The agent does NOT flag the fluent App.WaitForElement().Tap() chain as a missing-wait violation: 5/5 — The agent explicitly marked
WaitForElement("TestButton").Tap()as good practice and did not flag it as a violation. Perfect handling. - The convention compliance check passes or has no wait-related warnings for this code: 2/5 — The agent raised a wait-related warning about the FindElement call, saying it 'doesn't wait.' But the code does have WaitForElement("ResultLabel") on the line immediately before, meaning the convention of waiting before interaction IS satisfied. The agent's suggestion to consolidate is a style optimization, not a convention compliance fix, yet it was framed as a convention issue ('
⚠️ Issue'). The code should have received a pass for WaitForElement usage convention.
With-Skill Judge (Plugin)
Overall Score: 2.7/5
Reasoning: The agent does well on the critical first criterion — it correctly identifies the WaitForElement().Tap() chain as proper usage and does not flag it as a violation. However, it's overly critical on the second criterion, raising warnings about code that does follow the WaitForElement-before-interaction convention. The WaitForElement('ResultLabel') IS called before FindElement('ResultLabel'), so the wait convention is satisfied even if the return value isn't used. The race condition warning, while a thoughtful observation about test robustness, goes beyond what was asked (convention compliance regarding WaitForElement) and introduces a potentially non-existent API method. The analysis is competent and shows good understanding of UI testing patterns, but it over-diagnoses issues in code that is fundamentally convention-compliant.
- The agent does NOT flag the fluent App.WaitForElement().Tap() chain as a missing-wait violation: 5/5 — The agent correctly identifies
App.WaitForElement("TestButton").Tap()as valid with a ✅ mark, noting 'Correct — waits then chains interaction.' It does not flag this as a violation. - The convention compliance check passes or has no wait-related warnings for this code: 1.7/5 — The agent explicitly flags wait-related warnings: it marks the WaitForElement("ResultLabel") line as
⚠️ ('discards the returned element') and the FindElement line as ❌ ('redundant lookup and potential race'). The code DOES correctly use WaitForElement before the FindElement interaction — the wait happens on the immediately preceding line. The agent is being overly strict by requiring fluent chaining rather than recognizing that a separate WaitForElement call followed by FindElement on the same element ID is functionally correct and race-free (the element was confirmed present). The rubric expects no wait-related warnings, but the agent produced two.
Baseline Agent Output
`
The test is almost correct but has one issue on lines 3–4:
csharp App.WaitForElement("ResultLabel"); // waits, but discards the element var text = App.FindElement("ResultLabel").GetText(); // ❌ FindElement doesn't wait
FindElement does not wait — it's a point-in-time lookup that can be flaky. Since WaitForElement already returns the element, use its return value directly:
Fixed version:
csharp [Test] [Category(UITestCategories.Button)] public void ButtonUpdatesLabel() { App.WaitForElement("TestButton").Tap(); var text = App.WaitForElement("ResultLabel").GetText(); Assert.That(text, Is.EqualTo("Clicked")); }
Summary:
- ✅ Line 1 —
WaitForElementbefore.Tap()is correct - ❌ Lines 2–3 — Redundant
WaitForElement+ unsafeFindElement. Combine into a singleWaitForElement(...).GetText()call - ✅ Assertion style is fine
Rule of thumb: Never use App.FindElement — always use App.WaitForElement to get elements before any interaction or read.
`
With-Skill Agent Output (Isolated)
`
The test has one convention issue:
Line 3 is wrong — App.FindElement("ResultLabel") should not be used. The WaitForElement on line 2 waits but discards its result, then FindElement re-queries without waiting — a potential race condition.
Fix: Combine lines 2–3 by chaining off WaitForElement:
csharp [Test] [Category(UITestCategories.Button)] public void ButtonUpdatesLabel() { App.WaitForElement("TestButton").Tap(); var text = App.WaitForElement("ResultLabel").GetText(); Assert.That(text, Is.EqualTo("Clicked")); }
Rule: Always use WaitForElement before any element interaction — never FindElement. Line 1 (WaitForElement(...).Tap()) already follows this pattern correctly.
`
With-Skill Agent Output (Plugin)
`
The test has the right idea but has two issues:
1. Discarded WaitForElement result (minor):
Line 2 waits for "ResultLabel" but discards the result, then line 3 calls FindElement separately. Use the return value instead:
csharp var text = App.WaitForElement("ResultLabel").GetText();
2. Race condition on text content (flakiness risk):
WaitForElement("ResultLabel") only waits for the element to exist — it may already be present with empty/stale text before the tap's effect propagates. There's no guarantee the text is "Clicked" yet.
Corrected version:
csharp [Test] [Category(UITestCategories.Button)] public void ButtonUpdatesLabel() { App.WaitForElement("TestButton").Tap(); var text = App.WaitForElement("ResultLabel").GetText(); Assert.That(text, Is.EqualTo("Clicked")); }
Or, for maximum robustness against the race condition, poll for the expected text:
csharp App.WaitForElement("TestButton").Tap(); App.WaitForTextToBePresentInElement("ResultLabel", "Clicked");
Summary: The original correctly uses WaitForElement before .Tap() (✅), but the read path wastes the wait result and is potentially flaky if the label exists before the text updates.
`
📊 evaluate-pr-tests / happy-path-evaluate-pr-tests-and-produce-structured-report
Judge Report: Happy path - evaluate PR tests and produce structured report
Baseline Judge
Overall Score: 3/5
Reasoning: The agent produced a well-structured and specific test evaluation report with good coverage of quality criteria. The analysis is clearly informed by the actual diff content obtained via web_fetch, with specific code references throughout. However, the agent completely missed running the Gather-TestContext.ps1 script, which is an explicit rubric requirement. The agent also struggled with tooling (gh CLI auth failure, failed background agent, failed GitHub search), though it recovered by relying on the diff content obtained via web_fetch. The final report quality is good—specific, well-organized, and actionable—but the missing prerequisite script is a significant gap that prevents a higher score.
- The agent runs the Gather-TestContext.ps1 script to gather automated context before evaluating: 1/5 — The agent never ran the Gather-TestContext.ps1 script at any point in its session. It attempted gh CLI (failed due to missing token), then fell back to web_fetch calls to GitHub. The required script was completely ignored.
- The report covers all major criteria: Fix Coverage, Edge Cases, Test Type, Convention Compliance, Flakiness Risk: 5/5 — All five required criteria are explicitly covered in the evaluation table: Fix Coverage ([Draft] Readme WIP #1), Edge Cases (Update README.md #2), Test Type (Third #3), Convention Compliance (Aloha System.Maui! #4 as 'Conventions'), and Flakiness Risk (Update README.md #5). The report even goes beyond with additional criteria like Duplicate Coverage, Platform Scope, Assertion Quality, and Fix-Test Alignment.
- Each criterion has a verdict (pass/concern/fail) with a specific explanation, not just generic text: 4/5 — Each criterion has a clear verdict using ✅ Good /
⚠️ Gaps /⚠️ Minor symbols, and the explanations are specific to the actual code. For example, Fix Coverage references 'the Gone→Visible TabBar path', Edge Cases notes 'SecondTabLabel is defined but never exercised', and Flakiness Risk calls out 'ScrollTo() + VerifyScreenshot() can be device-sensitive'. These are concrete, code-specific observations, not boilerplate. The Key Findings section further elaborates with actionable specifics. Minor deduction because Convention Compliance's explanation about '#if ANDROID' being 'unconventional' could use more justification. - The Overall Verdict section summarizes the most important finding in 1-2 sentences: 4/5 — The 'Overall verdict' at the bottom provides a clear 2-sentence summary: 'Solid test that covers the primary fix path. The test type choice (UI test with screenshot) is appropriate. The main improvement opportunity is adding a second test case for repeated TabBar toggling and/or a programmatic position assertion as a complement to the screenshot.' It identifies the key finding (test is solid but could be expanded), though it's slightly more than 2 sentences.
With-Skill Judge (Isolated)
Overall Score: 3/5
Reasoning: The agent produced a high-quality, detailed evaluation report with specific, PR-relevant analysis across many dimensions. The test type assessment, convention compliance check, and flakiness risk analysis are all well-reasoned. However, the agent completely failed to run the Gather-TestContext.ps1 script, which is a fundamental process requirement. The agent's workaround (web scraping GitHub pages) was resourceful and ultimately yielded enough context for a good report, but skipping the prescribed automation step is a significant process failure that prevents a higher score.
- The agent runs the Gather-TestContext.ps1 script to gather automated context before evaluating: 1/5 — The agent never ran Gather-TestContext.ps1. It loaded the 'evaluate-pr-tests' skill (which presumably instructs using this script), but instead manually scraped GitHub via web_fetch after gh CLI failed due to token issues. The script was never attempted — not even a failed attempt to locate or execute it appears in the timeline.
- The report covers all major criteria: Fix Coverage, Edge Cases, Test Type, Convention Compliance, Flakiness Risk: 5/5 — All five required criteria are present as distinct, labeled sections: Fix Coverage (Section 1), Edge Cases & Gaps (Section 2), Test Type Appropriateness (Section 3), Convention Compliance (Section 4), and Flakiness Risk (Section 5). The report also goes beyond the minimum by adding Duplicate Coverage, Platform Scope, Assertion Quality, and Fix-Test Alignment sections.
- Each criterion has a verdict (pass/concern/fail) with a specific explanation, not just generic text: 4.7/5 — Each section has a clear verdict using ✅ (pass) or
⚠️ (concern) icons. Explanations are specific and grounded in actual code: e.g., Fix Coverage traces the exact Gone→Visible path through UpdateTabBarVisibility(); Edge Cases identifies 3 specific missing scenarios (repeated navigation, tab switching, detached view); Flakiness Risk identifies specific risky operations (ScrollTo, VerifyScreenshot without retryTimeout) with risk levels. The only minor gap is that verdicts don't use the exact pass/concern/fail terminology, but the intent is clear. - The Overall Verdict section summarizes the most important finding in 1-2 sentences: 4/5 — The Overall Verdict section is present and states 'Tests are adequate' followed by a 3-sentence explanation highlighting that the UI test directly exercises the fixed path and screenshot assertion is the right tool, with minor improvements possible. It's slightly over the 1-2 sentence ideal but is concise and captures the key finding well. The most important finding — that tests adequately cover the fix — is front and center.
With-Skill Judge (Plugin)
Overall Score: 3/5
Reasoning: The agent produced a thorough, well-structured, and technically detailed test evaluation report despite working with a sparse checkout. The report covers all required criteria and provides actionable recommendations. However, the agent completely missed the requirement to run the Gather-TestContext.ps1 script, which is a specific rubric criterion. The agent also wasted significant time (roughly 8 of 20 turns) trying to authenticate with GitHub CLI before falling back to web_fetch, and ultimately hit the 180-second timeout with a truncated final recommendation. The quality of the analysis itself is strong — it correctly identifies the fix-test alignment, names specific code elements, and raises valid concerns about screenshot timing — but the failure to follow the prescribed workflow (running the context-gathering script) and the inefficient execution path bring the overall score down.
- The agent runs the Gather-TestContext.ps1 script to gather automated context before evaluating: 1/5 — The agent never searched for, located, or ran any Gather-TestContext.ps1 script at any point in its execution. Instead, it manually gathered context through a combination of gh CLI attempts (which all failed due to auth issues), web_fetch calls to GitHub, and local git commands. While the agent did invoke the 'evaluate-pr-tests' skill which presumably provided instructions about this script, it completely skipped this step. This is a clear miss of the required automated context-gathering step.
- The report covers all major criteria: Fix Coverage, Edge Cases, Test Type, Convention Compliance, Flakiness Risk: 5/5 — The report covers all five required criteria and goes beyond them. It includes: (1) Fix Coverage, (2) Edge Cases & Gaps, (3) Test Type Appropriateness, (4) Convention Compliance, (5) Flakiness Risk. It additionally covers Duplicate Coverage, Platform Scope, Assertion Quality, and Fix-Test Alignment — providing a thorough multi-dimensional analysis of the test quality.
- Each criterion has a verdict (pass/concern/fail) with a specific explanation, not just generic text: 4.3/5 — Each criterion has a clear verdict using ✅ or
⚠️ icons, and explanations are grounded in the actual code. Fix Coverage references the specific methodUpdateTabBarVisibility()andRequestApplyInsets. Convention Compliance lists specific attributes like[Issue()],[Category()], and the_IssuesUITestbase class. Flakiness Risk provides a table with specific concerns like missingretryTimeoutonVerifyScreenshot()and unreliableApp.ScrollTo. Edge Cases lists four specific missing test scenarios. The explanations are mostly specific, though some could be even more code-grounded (e.g., Convention Compliance note about #if ANDROID could benefit from citing the project's documented convention). - The Overall Verdict section summarizes the most important finding in 1-2 sentences: 4/5 — The Overall Verdict states 'Tests are adequate — with minor concerns' and follows with a concise two-sentence summary: 'The UI test directly reproduces the bug scenario (navigate to detail → hide TabBar → go back → TabBar reappears) and uses screenshot verification to catch the visual regression. This is a well-chosen test for a layout/insets bug.' This effectively captures the key finding in a succinct manner. It could be slightly improved by also mentioning the most important concern (e.g., flakiness risk from missing retryTimeout).
Baseline Agent Output
`
GitHub search requires auth. I have enough information from the diff to do a thorough evaluation. Here's my assessment:
PR #34324 — Test Evaluation Report
PR: Fix empty space above TabBar after navigating back when TabBar visibility is toggled
Fix: ShellItemRenderer.cs (Android) — re-requests window insets on Gone→Visible TabBar transition
Files: 1 fix file, 1 UI test, 1 HostApp page, 1 reference snapshot
1. Fix Coverage — ✅ Good
The test directly exercises the fix's code path: navigate to a detail page (hides TabBar → Gone), press Back (shows TabBar → Visible), then scrolls to the bottom label and takes a screenshot. This hits exactly the wasGone && showTabs branch in UpdateTabBarVisibility().
2. Edge Cases & Gaps — ⚠️ Minor gaps
- ✅ Core happy path covered (single forward+back navigation)
- ❌ No test for multiple rapid toggles (navigate→back→navigate→back)
- ❌ No test for tab switching after navigating back (could reveal stale insets on the second tab)
- ℹ️ The fix's
IsAttachedToWindowguard isn't stress-tested, though it's defensive code
These are minor — the primary regression scenario is well-covered.
3. Test Type — ✅ Appropriate
A UI test with screenshot verification is the correct choice here. The bug is a visual layout issue (empty space from stale safe-area padding). This cannot be caught by:
- Unit tests (no Android view system)
- XAML tests (not a XAML parsing issue)
- Device tests (could check bounds, but screenshot is more reliable for visual regressions)
4. Convention Compliance — ✅ / ⚠️ Minor
- ✅
[Issue(IssueTracker.Github, 33703, ...)]attribute present - ✅ Extends
_IssuesUITestbase class - ✅
[Category(UITestCategories.Shell)]applied - ✅ File naming follows
Issue{number}.cspattern - ✅ Snapshot in correct directory (
TestCases.Android.Tests/snapshots/android/) ⚠️ Uses#if ANDROIDwrapping the entire file — this is a common pattern inTestCases.Shared.Testsfor platform-specific issues, so acceptable, though placing it inTestCases.Android.Testswould be slightly cleaner
5. Flakiness Risk — ✅ Low
- ✅
WaitForElement()used before every interaction - ✅
ScrollTo()has built-in waiting - ✅ No arbitrary
Thread.Sleep()orTask.Delay() - ℹ️ The fix uses
platformView.Post()(async), but theScrollTo("BottomLabel")afterWaitForElement("NavigateButton")provides sufficient time for the posted callback to complete
6. Duplicate Coverage — ✅ No duplication
This tests a specific visual regression (stale bottom padding after TabBar reappears). No existing test covers this exact Gone→Visible inset recalculation scenario.
7. Platform Scope — ✅ Correct
- Fix is Android-only (
ShellItemRenderer.csunderShell/Android/) - Test is Android-only (
#if ANDROID) - PR author confirmed manual testing on all 4 platforms
8. Assertion Quality — ✅ Strong
VerifyScreenshot() is the strongest possible assertion for this visual bug — it captures the exact pixel state and will detect any regression in padding/spacing.
9. Fix-Test Alignment — ✅ Direct
Test and fix target the exact same code path: UpdateTabBarVisibility() handling the Gone → Visible transition.
Summary
| Criterion | Rating |
|---|---|
| Fix Coverage | ✅ Good |
| Edge Cases | |
| Test Type | ✅ Appropriate |
| Conventions | ✅ Compliant |
| Flakiness | ✅ Low risk |
| Duplicates | ✅ None |
| Platform Scope | ✅ Correct |
| Assertions | ✅ Strong |
| Fix-Test Alignment | ✅ Direct |
Overall: Good quality. The test is well-structured, uses the right test type, follows conventions, and directly exercises the fix. The only suggestion would be an additional test for repeated toggle cycles to guard against cumulative inset drift, but the primary regression is solidly covered.
`
With-Skill Agent Output (Isolated)
`
Now I have everything needed. Here's the full evaluation report:
PR Test Evaluation Report
PR: #34324 — Empty space above TabBar after navigating back when TabBar visibility is toggled
Test files evaluated: 3 (1 UI test, 1 HostApp page, 1 snapshot)
Fix files: 1 (ShellItemRenderer.cs)
Overall Verdict
✅ Tests are adequate
The UI test directly exercises the Gone→Visible TabBar transition that the fix addresses, and the screenshot assertion is well-suited for catching this visual regression. Minor improvements possible.
1. Fix Coverage — ✅
The fix adds a ViewCompat.RequestApplyInsets() call when the TabBar transitions from Gone to Visible. The test navigates to a detail page (which sets TabBarIsVisible = false, making it Gone), then navigates back (making it Visible again), then scrolls to the bottom and takes a screenshot. This directly triggers the fixed code path.
2. Edge Cases & Gaps — ⚠️ Concern
Covered:
- Single forward/back navigation with TabBar toggle
Missing:
- Multiple toggle cycles — Navigate to detail, back, detail, back again. The fix uses
Post()for deferred execution; multiple rapid transitions could reveal race conditions. - Tab switching after navigating back — Switching to "Second Page" tab after the back navigation could interact with the fix.
- Rapid navigation — Quick tap on navigate + immediate back could hit the
IsAttachedToWindowguard.
3. Test Type Appropriateness — ✅
Current: UI Test with VerifyScreenshot()
Recommendation: Appropriate. The bug is a visual layout issue (empty space above TabBar) caused by stale window insets on Android. This requires real platform rendering, actual Shell navigation, and Android's inset system — none of which can be tested with unit or XAML tests. A device test could check layout bounds programmatically, but the screenshot approach catches the exact visual regression users reported.
4. Convention Compliance — ⚠️ Concern
- ✅ File naming:
Issue33703.cs - ✅
[Issue()]attribute on HostApp page - ✅
[Category(UITestCategories.Shell)]on test method - ✅
_IssuesUITestbase class - ✅
WaitForElementbefore all interactions - ✅ No
Task.Delay/Thread.Sleep ⚠️ Inline#if ANDROIDpreprocessor directive wraps the entire test file. While this is functionally correct (bug is Android-only), it's a flagged convention concern. However, this pattern is common inTestCases.Shared.Testsfor platform-specific issues, so it's an acceptable deviation.
5. Flakiness Risk — ⚠️ Medium
⚠️ App.ScrollTo("BottomLabel")— scroll operations can be unreliable in CI, especially in ScrollView containers.⚠️ VerifyScreenshot()withoutretryTimeout— no retry buffer for rendering latency after the layout recalculation. AddingretryTimeout: TimeSpan.FromSeconds(2)would reduce flakiness.- ✅
WaitForElementused before every interaction. - ✅ No arbitrary delays.
6. Duplicate Coverage — ✅ No duplicates
This tests a specific regression in Shell TabBar visibility toggling with inset recalculation. While other Shell tests exist, none specifically cover the Gone→Visible TabBar transition's effect on safe-area padding.
7. Platform Scope — ✅
Fix is in Android/ShellItemRenderer.cs (Android-only). Test is wrapped in #if ANDROID. The issue was reported as Android-only (above Android 13), and the PR author verified no issues on iOS/Windows/Mac. Platform scope is correct.
8. Assertion Quality — ✅
VerifyScreenshot() is a strong assertion for this visual layout bug. It compares the rendered bottom of the page against a reference snapshot, which directly captures whether the empty space exists above the TabBar. This is more robust than a bounds-check assertion for catching the exact user-visible regression.
9. Fix-Test Alignment — ✅
Fix modifies UpdateTabBarVisibility() in ShellItemRenderer.cs to re-request insets on Gone→Visible transition. Test creates a Shell with TabBar, navigates to a page that hides the TabBar, navigates back (triggering the transition), and verifies the visual result. Direct alignment.
Recommendations
- Add
retryTimeouttoVerifyScreenshot()—VerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2))would reduce flakiness from rendering latency after the inset recalculation. - Consider a second test for multiple toggles — A test that navigates forward/back twice would cover potential race conditions in the
Post()deferred execution. - Minor:
App.ScrollToreliability — If CI flakiness is observed, consider usingApp.ScrollDown()in a loop withWaitForElementas a fallback pattern.
With-Skill Agent Output (Plugin)
`
The repo has a minimal checkout without source files. I have the complete diff from GitHub though. Let me fetch the linked issue and check for any existing similar tests via the GitHub web interface.I have all the information needed. Let me compile the full evaluation.
PR Test Evaluation Report
PR: #34324 — Empty space above TabBar after navigating back when TabBar visibility is toggled
Test files evaluated: 3 (1 HostApp page, 1 UI test, 1 snapshot)
Fix files: 1 (ShellItemRenderer.cs — Android)
Overall Verdict
✅ Tests are adequate — with minor improvements recommended
The UI test directly reproduces the bug scenario (navigate to detail → hide TabBar → navigate back → verify no empty space) and uses screenshot verification, which is the right approach for a visual layout bug. A few edge cases and a flakiness concern are noted below.
1. Fix Coverage — ✅
The test exercises the exact code path: TabBar transitions from Gone → Visible when navigating back from a detail page where Shell.TabBarIsVisible = false. The VerifyScreenshot() captures the visual state at the point where the fix's ViewCompat.RequestApplyInsets() call has effect. Reverting the fix would cause the screenshot to show empty space above the TabBar, failing the test.
2. Edge Cases & Gaps — ⚠️
Covered:
- Single navigate forward (hide TabBar) → back (show TabBar) round-trip
- ScrollView content extending to the bottom edge
Missing:
- Multiple round-trips — navigate → back → navigate → back. Could repeated transitions accumulate stale padding?
- Tab switching after return — does switching tabs after returning cause layout issues?
- Rapid navigation — the fix guards with
IsAttachedToWindow, but no test verifies the safety of fast back navigation beforePost()fires - TabBar already visible — no test for the
wasGoneguard (ensuring no unnecessary inset requests)
3. Test Type Appropriateness — ✅
Current: UI Test with screenshot verification
Recommendation: Correct choice. This bug is purely visual (empty space in layout) and involves Android platform rendering behavior triggered by navigation. Screenshot comparison is the most reliable way to verify absence of the empty space. Cannot be tested with unit or device tests.
4. Convention Compliance — ⚠️
| Check | Status |
|---|---|
File naming IssueXXXXX.cs |
✅ |
[Issue()] attribute on HostApp |
✅ |
_IssuesUITest base class |
✅ |
[Category(UITestCategories.Shell)] |
✅ |
WaitForElement before interactions |
✅ |
No Task.Delay/Thread.Sleep |
✅ |
Inline #if ANDROID |
TestCases.Shared.Tests for platform-specific tests. |
5. Flakiness Risk — ⚠️ Medium
| Risk Factor | Details |
|---|---|
VerifyScreenshot() without retryTimeout |
The fix uses platformView.Post() (async). Screenshot may be captured before insets are re-applied. Recommend adding retryTimeout: TimeSpan.FromSeconds(2) |
App.ScrollTo("BottomLabel") |
Scroll behavior can vary across emulator configurations |
this.Back() timing |
Back navigation followed immediately by WaitForElement — mitigated by WaitForElement's built-in timeout |
6. Duplicate Coverage — ✅ No duplicates
No existing tests cover the specific TabBar Gone → Visible transition and its effect on page layout/safe-area padding. This is a unique scenario.
7. Platform Scope — ✅
Fix is Android-only (ShellItemRenderer.cs in Android/ folder). Test is Android-only (#if ANDROID). Issue confirmed Android-only by reporter (above Android 13). PR author verified no impact on iOS/Windows/Mac. ✅ Correct alignment.
8. Assertion Quality — ⚠️
- ✅
VerifyScreenshot()is appropriate for visual layout bugs — catches the exact empty space ⚠️ No programmatic assertion on element positions (e.g., verifyingBottomLabelrect is adjacent to TabBar)⚠️ Screenshot-only assertion is fragile if unrelated visual changes occur (but this is inherent to screenshot tests and acceptable)
A supplementary positional assertion like checking BottomLabel's Y + Height relative to the screen bottom would make the test more robust, but the screenshot approach is standard for this type of issue.
9. Fix-Test Alignment — ✅
| Fix | Test |
|---|---|
ShellItemRenderer.UpdateTabBarVisibility() — re-requests insets on Gone→Visible |
Navigates to page that hides TabBar, then navigates back (triggers Visible transition) |
| Android-specific code path | #if ANDROID test |
| ScrollView + safe-area padding interaction | HostApp uses ScrollView with 15 rows extending to bottom |
Direct alignment. The test scenario is a faithful reproduction of the reported bug.
Recommendations
-
Add
retryTimeouttoVerifyScreenshot()— The fix usesplatformView.Post()which is asynchronous. AddingretryTimeout: TimeSpan.FromSeconds(2)would protect against screenshot timing issues:
csharp VerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2)); -
Consider a repeated-navigation test — Add a second test method or extend the existing one to do navigate → back → navigate → back to verify the
📊 evaluate-pr-tests / near-miss-recall-informal-test-quality-check
Judge Report: Near-miss recall - informal test quality check
Baseline Judge
Overall Score: 1/5
Reasoning: The agent performed a competent informal review of the PR test files — it correctly identified the 4 files changed, understood the bug being fixed (empty space above TabBar on Android), and made reasonable observations about the test quality (proper conventions, WaitForElement usage, screenshot verification, correct #if ANDROID guard). However, it completely failed to meet both rubric criteria: it did not invoke the expected 'evaluate-pr-tests' structured workflow, and it did not produce the required 9-criteria structured report. Instead it delivered a casual narrative review. The technical content is decent, but the format and process are entirely wrong per the rubric's expectations.
- The agent invokes the evaluate-pr-tests structured workflow even for informal phrasing: 1/5 — The agent did not invoke any structured workflow or skill for evaluating PR tests. It performed ad-hoc investigation using bash, web_fetch, and report_intent, but never triggered a dedicated 'evaluate-pr-tests' workflow. The agent treated this as a freeform research task rather than recognizing it as a structured evaluation request.
- The agent produces the structured 9-criteria report, not just a casual opinion: 1/5 — The agent produced a casual, informal assessment with bullet-pointed strengths and minor observations. There is no structured 9-criteria report — just a general verdict ('Reasonable ✅') with ad-hoc commentary. The output lacks any formal scoring framework, numbered criteria, or systematic evaluation structure that a 9-criteria report would entail.
With-Skill Judge (Isolated)
Overall Score: 4.7/5
Reasoning: The agent performed excellently. It immediately recognized the task as a PR test evaluation despite informal phrasing, invoked the correct skill, efficiently gathered all necessary data (PR metadata, diff, test files, fix file) via web API after the gh CLI wasn't available, and produced a comprehensive 9-criteria structured report with specific, code-grounded analysis. The recommendations (e.g., adding retryTimeout to VerifyScreenshot, fixing the leading space on the preprocessor directive, suggesting repeated navigation cycle tests) are actionable and well-reasoned. The entire process was completed efficiently in 5 turns with no errors.
- The agent invokes the evaluate-pr-tests structured workflow even for informal phrasing: 5/5 — The user asked informally ('tell me if they seem reasonable'), and the agent immediately invoked the 'evaluate-pr-tests' skill as one of its very first actions alongside the initial report_intent call. This demonstrates proper recognition that the informal request maps to the structured PR test evaluation workflow.
- The agent produces the structured 9-criteria report, not just a casual opinion: 4.7/5 — The output contains a well-structured report with exactly 9 numbered criteria: (1) Fix Coverage, (2) Edge Cases & Gaps, (3) Test Type Appropriateness, (4) Convention Compliance, (5) Flakiness Risk, (6) Duplicate Coverage, (7) Platform Scope, (8) Assertion Quality, (9) Fix-Test Alignment. Each criterion has a clear verdict icon (✅/
⚠️ ), substantive analysis grounded in the actual file content, and the report includes an Overall Verdict header plus actionable Recommendations. This is far more than a casual opinion — it's a rigorous structured evaluation.
With-Skill Judge (Plugin)
Overall Score: 4.7/5
Reasoning: The agent performed excellently on both rubric criteria. It immediately recognized the informal request as a PR test evaluation task and invoked the appropriate skill. It then produced a comprehensive, well-structured 9-criteria report with specific, substantive analysis of each criterion. The agent also showed good adaptability when its primary tools (gh CLI, local grep) failed, falling back to the GitHub REST API to gather the needed information. The final report demonstrates genuine understanding of the PR's code changes, test patterns, and MAUI testing conventions. The recommendations (adding retryTimeout, testing repeated toggles) are practical and relevant. Minor gap: the duplicate coverage check was incomplete due to tooling limitations, but the agent acknowledged this and still provided a reasonable assessment.
- The agent invokes the evaluate-pr-tests structured workflow even for informal phrasing: 5/5 — The agent immediately invoked the 'evaluate-pr-tests' skill on its very first tool-calling turn, before doing anything else. The user's phrasing was informal ('tell me if they seem reasonable') and the agent correctly recognized this as a PR test evaluation task and triggered the structured workflow.
- The agent produces the structured 9-criteria report, not just a casual opinion: 5/5 — The output contains a clearly structured report with all 9 criteria: (1) Fix Coverage, (2) Edge Cases & Gaps, (3) Test Type Appropriateness, (4) Convention Compliance, (5) Flakiness Risk, (6) Duplicate Coverage, (7) Platform Scope, (8) Assertion Quality, (9) Fix-Test Alignment. Each criterion has a verdict icon (✅/
⚠️ ), a heading, and substantive analysis. The report also includes an Overall Verdict section and actionable Recommendations. This is a well-structured professional report, not a casual opinion.
Baseline Agent Output
`
Here's my assessment of the test files in PR #34324:
PR #34324 — Test Review
Issue: Empty space above TabBar after navigating back when TabBar visibility is toggled (Android-only)
Files Changed (4 total)
| File | Purpose |
|---|---|
ShellItemRenderer.cs |
Fix — re-requests window insets after Gone→Visible TabBar transition |
Issue33703.cs (HostApp) |
Test page — Shell with tabs + detail page that hides TabBar |
Issue33703.cs (Shared.Tests) |
UI test — navigates, goes back, scrolls, screenshot-verifies |
*.png (snapshot) |
Baseline — expected screenshot |
Verdict: ✅ Tests look solid
Strengths:
- Directly exercises the bug path: Navigate → TabBar hidden → Back → TabBar reappears → verify no empty space
- Correct
#if ANDROIDguard with clear explanatory comment - Proper conventions:
_IssuesUITestbase class,[Issue]attribute,[Category(UITestCategories.Shell)] WaitForElementbefore every interaction — no flaky arbitrary delays- Screenshot verification catches the visual regression (empty padding above TabBar)
- Scrolls to bottom before screenshot, which is exactly where the empty space would appear
Minor observations (not concerns):
- Some HostApp AutomationIds (
SecondTabLabel,MainScrollView,InstructionLabel) aren't used by the test — fine for debugging, no issue - Only tests one navigation cycle — sufficient to catch the bug, though a repeated cycle could be more thorough
- No unit test alternative exists here — this is inherently an Android layout/insets issue that requires a device UI test
Overall, the test closely mirrors the reported bug scenario, follows MAUI test conventions, and uses screenshot verification to catch the exact visual defect. Well done.
`
With-Skill Agent Output (Isolated)
`
PR Test Evaluation Report
PR: #34324 — [Android] Fix empty space above TabBar after navigating back when TabBar visibility is toggled
Test files evaluated: 3 (1 HostApp page, 1 UI test, 1 snapshot)
Fix files: 1 (ShellItemRenderer.cs)
Overall Verdict
✅ Tests are adequate
The UI test directly exercises the Gone→Visible TabBar transition that the fix addresses, and the screenshot assertion is a strong verification method for a visual layout bug.
1. Fix Coverage — ✅
The test navigates to a detail page (which sets Shell.SetTabBarIsVisible(this, false) → Gone), presses Back (→ Visible), then scrolls to the bottom and takes a screenshot. This directly triggers the wasGone && showTabs code path added in the fix.
2. Edge Cases & Gaps — ⚠️
Covered:
- Single forward/back navigation with TabBar visibility toggle
Missing:
- Multiple navigation cycles (navigate → back → navigate → back) to check repeated transitions
- Tab switching after navigating back (does the second tab also render correctly?)
3. Test Type Appropriateness — ✅
Current: UI Test with screenshot verification
Recommendation: Correct choice. This is a visual layout bug (empty space between content and TabBar) that can only be reliably verified via screenshot comparison. Cannot be tested at a lighter level.
4. Convention Compliance — ⚠️
- ✅ File naming:
Issue33703.cs - ✅
[Issue()]attribute on HostApp page withPlatformAffected.Android - ✅
_IssuesUITestbase class - ✅
[Category(UITestCategories.Shell)]— one category on method - ✅
WaitForElementbeforeTap - ✅ No
Task.Delay/Thread.Sleep ⚠️ Leading space before#if ANDROIDdirective (line 1:#if ANDROID) — cosmetic but unconventional; preprocessor directives should start at column 0
5. Flakiness Risk — ⚠️ Medium
VerifyScreenshot()is called withoutretryTimeout— the layout may not have fully settled after scrolling, especially since the fix itself usesPost()for deferred inset applicationApp.ScrollTo("BottomLabel")can be timing-sensitive on slower CI devices
6. Duplicate Coverage — ✅ No duplicates
No existing tests cover TabBar visibility toggling after navigation in Shell.
7. Platform Scope — ✅
Fix is Android-only (ShellItemRenderer.cs under Android/), test is Android-only (#if ANDROID), snapshot is in TestCases.Android.Tests/snapshots/android/. Correct scoping.
8. Assertion Quality — ✅
VerifyScreenshot() is the right assertion here — it catches the exact visual regression (empty space above TabBar) by comparing against a known-good baseline.
9. Fix-Test Alignment — ✅
Fix modifies ShellItemRenderer.UpdateTabBarVisibility() for Gone→Visible transitions. Test triggers exactly that code path via Shell.SetTabBarIsVisible(this, false) on the detail page followed by a back navigation.
Recommendations
- Add
retryTimeouttoVerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2))to reduce flakiness risk from the deferredPost()inset recalculation - Fix leading space on
#if ANDROIDdirective (line 1 of the test file) - Consider adding a second test case for repeated navigation cycles (navigate → back → navigate → back) to ensure the fix holds under repeated transitions
With-Skill Agent Output (Plugin)
`
Here's my evaluation:
PR Test Evaluation Report
PR: #34324 — [Android] Fix empty space above TabBar after navigating back when TabBar visibility is toggled
Test files evaluated: 2 (HostApp page + UI test)
Fix files: 1 (ShellItemRenderer.cs)
Overall Verdict
✅ Tests are adequate
The UI test directly reproduces the bug scenario (navigate to detail → hide TabBar → go back → verify no empty space) and uses screenshot verification, which is the right approach for a layout/visual defect.
1. Fix Coverage — ✅
The test navigates to a detail page (which sets TabBarIsVisible = false), then goes back (triggering the Gone → Visible transition in UpdateTabBarVisibility()), and scrolls to the bottom to verify no empty space via VerifyScreenshot(). This directly exercises the patched code path.
2. Edge Cases & Gaps — ⚠️ Minor
Covered: Primary flow (forward → back → verify layout)
Missing:
- Repeated navigation: Navigate forward/back multiple times to check the fix holds across repeated toggles
- Tab switching after return: Switch to "Second Page" tab after navigating back to verify tabs still work correctly
These are minor — the core regression is well-covered.
3. Test Type Appropriateness — ✅
Current: UI Test
Recommendation: Correct choice. The bug is about visible empty space caused by stale window insets — this requires a real Android device/emulator and screenshot comparison. No lighter test type can catch this.
4. Convention Compliance — ✅
- ✅ File named
Issue33703.cs - ✅
[Issue()]attribute on HostApp page with correct tracker/ID - ✅
[Category(UITestCategories.Shell)]on test method - ✅
_IssuesUITestbase class - ✅
WaitForElementbefore all interactions - ✅ No
Task.Delay/Thread.Sleep - ✅
#if ANDROIDwrapper (appropriate — Android-only bug)
5. Flakiness Risk — ⚠️ Low-Medium
VerifyScreenshot() is called without retryTimeout. After scrolling and layout recalculation, a brief retryTimeout: TimeSpan.FromSeconds(2) would reduce flakiness risk in CI. Otherwise the test structure is solid.
6. Duplicate Coverage — ✅ No duplicates likely
The test targets a specific regression (inset reapplication after TabBar Gone→Visible), which is distinct from general Shell TabBar visibility tests.
7. Platform Scope — ✅
Fix is Android-only (ShellItemRenderer.cs in Android/ folder). Test is Android-only (#if ANDROID). Perfect alignment.
8. Assertion Quality — ✅
VerifyScreenshot() is the right assertion — it catches the exact visual defect (empty space above TabBar). Reference screenshot provides a concrete baseline.
9. Fix-Test Alignment — ✅
Fix modifies ShellItemRenderer.UpdateTabBarVisibility(). Test triggers that exact method by navigating to a page with Shell.SetTabBarIsVisible(this, false) and then going back. Directly aligned.
Recommendations
- Add
retryTimeouttoVerifyScreenshot()to reduce CI flakiness —VerifyScreenshot(retryTimeout: TimeSpan.FromSeconds(2)) - (Nice-to-have) Consider a second test case that navigates forward/back twice to validate repeated toggles don't regress
📊 evaluate-pr-tests / negative-trigger-general-code-review-should-not-produce-test-evaluation-report
Judge Report: Negative trigger - general code review should not produce test evaluation report
Baseline Judge
Overall Score: 3/5
Reasoning: The agent took the correct approach (general code review via git commands, no skill invocation) and correctly identified that the workspace was empty. It handled the failure gracefully by asking for repository information. However, it ultimately could not deliver any actual code review since there was no code to review. The agent's process was efficient - it made 3 parallel git commands on the first turn, quickly diagnosed the issue, and then confirmed with a directory listing. From the rubric's perspective, it clearly meets both criteria: it used a general review approach (not a skill workflow) and did not produce a 9-criteria evaluation structure. The score of 3 reflects that while the approach was correct and the criteria were met, no substantive review output was produced.
- The agent performs a general code review without invoking the evaluate-pr-tests skill workflow: 3.7/5 — The agent did attempt a general code review approach using standard git commands (git log, git diff) rather than invoking any skill workflow. It never called the evaluate-pr-tests or any other skill. However, it could not complete the actual review because the working directory was empty. It handled the missing repo gracefully but didn't try alternative locations (e.g., home directory, /home/runner, etc.) to find a possible repo. The approach was correct in spirit — general code review, no skill invocation — but incomplete in execution.
- The agent does not produce the 9-criteria evaluation structure from evaluate-pr-tests: 5/5 — The agent did not produce any 9-criteria evaluation structure. Its output was a simple message explaining the empty directory and asking for more information. This criterion is fully satisfied.
With-Skill Judge (Isolated)
Overall Score: 3/5
Reasoning: The agent took a reasonable and efficient approach: it checked for a git repository, found the working directory empty, and promptly asked the user for clarification. It correctly avoided invoking any specialized skill workflow and did not produce a structured evaluation format. The agent was methodical (2 targeted bash commands) and communicated clearly. However, it ultimately could not complete the core task (a code review) because there was no repository present, which limits the overall quality of the output. The response is acceptable given the constraints but doesn't demonstrate actual code review capability.
- The agent performs a general code review without invoking the evaluate-pr-tests skill workflow: 3.7/5 — The agent attempted to perform a general code review by directly using git commands (git log, git branch, git diff) rather than invoking the evaluate-pr-tests skill. This is the correct approach per the criterion. However, the agent was unable to actually complete any code review because the working directory was empty. It correctly identified the absence of a repository and asked for clarification, but no actual review was produced.
- The agent does not produce the 9-criteria evaluation structure from evaluate-pr-tests: 5/5 — The agent did not produce any 9-criteria evaluation structure. Its output was simply a message explaining that no git repository was found and asking for more information. This criterion is fully met, though largely by default since no review was produced at all.
With-Skill Judge (Plugin)
Overall Score: 3/5
Reasoning: The agent took a reasonable and efficient approach: it checked for a git repository, found the directory empty, and appropriately asked for more information. It correctly avoided using the evaluate-pr-tests skill and didn't fabricate a review. However, no actual code review was delivered since there was no code to review. The agent's behavior was sensible given the empty workspace, but the task ultimately went unfinished. The approach was methodical (2 targeted bash commands) and the failure was graceful.
- The agent performs a general code review without invoking the evaluate-pr-tests skill workflow: 3.3/5 — The agent did not invoke any skill workflow (including evaluate-pr-tests), and its approach was to directly check git history for a general code review. However, it ultimately did not perform any actual code review because the working directory was empty. It took a general approach in principle, but produced no review output.
- The agent does not produce the 9-criteria evaluation structure from evaluate-pr-tests: 5/5 — The agent produced no 9-criteria evaluation structure whatsoever. Its output was simply a message explaining the empty directory and asking for clarification. This criterion is fully satisfied.
Baseline Agent Output
`
The working directory is empty — there's no git repository or code here. Could you provide:
- A repository URL (e.g.,
https://github.com/user/repo) to clone, or - A PR number and repo (e.g.,
dotnet/maui#12345) to review?
With-Skill Agent Output (Isolated)
`
The directory is empty — there's no git repository here. Could you provide:
- The repository URL (e.g.,
https://github.com/owner/repo), or - The branch name and repo to clone?
I'll then check out the code and review the latest commit for you.
`
With-Skill Agent Output (Plugin)
`
The directory is empty and not a git repository. Could you provide:
- A repository URL (e.g.,
https://github.com/owner/repo) to clone, or - A PR number and repo to review?
I need a codebase to review the latest commit.
`
📊 evaluate-pr-tests / no-tests-added-pr-only-has-fix-files
Judge Report: No tests added - PR only has fix files
Baseline Judge
Overall Score: 3.3/5
Reasoning: The agent gave up too easily after finding the working directory empty. It made only 2 tool calls and didn't try alternative approaches like using gh CLI, searching the filesystem more broadly, or attempting to clone the repo. While it correctly identified the core issue (no tests added for source changes), its analysis is entirely surface-level and generic — it just restated what the user already told it (no test files were added) rather than providing meaningful evaluation. The response is more of a request for clarification than an actual evaluation.
- The agent flags the absence of tests as a Fix Coverage failure: 3.7/5 — The agent explicitly flags test coverage as missing with a bold '❌ Missing' label and states 'No test files were added or modified.' It further recommends the PR should not merge without tests. It doesn't use the exact phrase 'Fix Coverage failure' but the intent is unmistakably clear. A slightly deeper investigation into existing test patterns for CollectionView would have elevated this to a 5.
- The overall verdict reflects that no tests were added: 3.7/5 — The overall verdict is clear and unambiguous: the PR lacks tests and 'should not be merged without tests.' The agent provides a helpful table of what test types should be considered based on the change type. The verdict accurately reflects the absence of test files. It could have been more specific about what tests exist for CollectionView already, but the core message is strong.
With-Skill Judge (Isolated)
Overall Score: 4/5
Reasoning: The agent correctly and efficiently identified that no tests were added to the PR, flagged it as a fix coverage failure, and provided a clear overall verdict. The recommendations are reasonable (unit tests, device tests, revert-to-verify approach). It could have gone further by examining the actual code changes to provide more specific and actionable test recommendations (e.g., what specific properties or behaviors to test), but for the given rubric criteria, the output is well-structured, accurate, and concise.
- The agent flags the absence of tests as a Fix Coverage failure: 5/5 — The agent explicitly marks 'Fix Coverage — ❌' and clearly states that no tests were added, there is no automated verification of the changed code paths, and there is no way to prevent regressions. This is a direct and unambiguous flagging of the absence of tests as a fix coverage failure.
- The overall verdict reflects that no tests were added: 5/5 — The overall verdict is clearly stated as '❌ Tests are insufficient' with the explanation 'No test files were added or modified in this PR. The fix changes CollectionView.cs and CollectionViewHandler.cs but includes zero test coverage.' This directly and accurately reflects the absence of tests.
With-Skill Judge (Plugin)
Overall Score: 4/5
Reasoning: The agent correctly identified the core issue — no tests were added for changes to two source files — and produced a well-structured, actionable evaluation report. It flagged Fix Coverage as a failure, gave a clear overall verdict, and provided sensible recommendations (unit tests, device tests, UI tests as appropriate, verifying test fails without fix, edge cases). The agent tried to examine the actual source files but couldn't find them; despite this, its conclusion was correct and well-reasoned based on the prompt information. The only minor shortcoming is that it couldn't inspect the actual code changes to provide more specific test recommendations, but this was due to file availability rather than agent error.
- The agent flags the absence of tests as a Fix Coverage failure: 5/5 — The agent clearly flags the absence of tests under a dedicated 'Fix Coverage — ❌' section, explicitly stating 'No tests were added. The code changes in CollectionView.cs and CollectionViewHandler.cs are completely untested by this PR.' This is unambiguous and directly addresses the criterion.
- The overall verdict reflects that no tests were added: 5/5 — The overall verdict is clearly stated as '❌ Tests are insufficient' with the supporting explanation 'No test files were added or modified in this PR. Changes to CollectionView and its handler require test coverage to prevent regressions.' This fully reflects that no tests were added.
Baseline Agent Output
`
The working directory appears empty — no repository is cloned here. Could you provide the PR number or repository URL so I can check out the code and evaluate the changes? For example:
gh pr view <PR_NUMBER> --repo dotnet/maui- Or a direct link like
https://github.com/dotnet/maui/pull/XXXXX
That said, based on what you've described:
🚩 Missing Tests Flag
Two source files were changed (CollectionView.cs and CollectionViewHandler.cs) with zero test files added or modified. This is a concern — functional changes to core controls and handlers should have corresponding tests.
Relevant test projects for these files would be:
src/Controls/tests/Core.UnitTests/— unit tests forCollectionView.cs- Device tests for handler behavior (
CollectionViewHandler.cs)
If you share the PR number, I can clone the repo, inspect the actual diffs, and give a detailed assessment of what specific tests are missing.
`
With-Skill Agent Output (Isolated)
`
PR Test Evaluation Report
Test files evaluated: 0
Fix files: 2 (CollectionView.cs, CollectionViewHandler.cs)
Overall Verdict
❌ Tests are insufficient
No test files were added or modified in this PR. Changes to CollectionView and its handler are untested.
1. Fix Coverage — ❌
No tests were added. The code changes in CollectionView.cs and CollectionViewHandler.cs have zero test coverage from this PR.
Recommendations
- Add tests — At minimum, unit tests in
Controls.Core.UnitTestsfor any property/logic changes inCollectionView.cs. If the fix involves platform rendering or native handler behavior, device tests are appropriate. UI tests should only be used if the fix requires verifying end-to-end user interaction. - Verify fix coverage — Tests should fail when the fix is reverted and pass with it applied, confirming they exercise the actual bug.
With-Skill Agent Output (Plugin)
`
PR Test Evaluation Report
Test files evaluated: 0
Fix files: 2
src/Controls/src/Core/CollectionView.cssrc/Controls/src/Core/Handlers/CollectionViewHandler.cs
Overall Verdict
❌ Tests are insufficient
No test files were added or modified in this PR. Changes to CollectionView and its handler affect core control behavior and must have test coverage to prevent regressions.
1. Fix Coverage — ❌
No tests were added. The fix is completely uncovered. Per evaluation policy, remaining criteria are skipped when no tests exist.
Recommendations
- Add tests — Determine what changed and add the lightest appropriate test type:
- Property/logic changes in
CollectionView.cs→ unit test inControls.Core.UnitTests - Handler mapping/wiring in
CollectionViewHandler.cs→ device test inControls.DeviceTests - User-facing interaction or layout bugs → UI test in
TestCases.Shared.Tests(as a last resort)
- Property/logic changes in
- Verify the test fails without the fix — Revert the fix, confirm the test reproduces the bug, then re-apply.
- Cover edge cases — Especially null inputs, empty collections, repeated operations, and platform-specific paths if the handler change is platform-targeted.
📊 evaluate-pr-tests / test-type-downgrade-recommendation-ui-test-for-pure-property-logic
Judge Report: Test type downgrade recommendation - UI test for pure property logic
Baseline Judge
Overall Score: 5/5
Reasoning: The agent delivered an excellent, concise, and accurate evaluation with zero tool calls needed — the task was purely analytical and the agent correctly treated it as such. The answer correctly identifies the problem (UI test is overkill for a property setter change), explains why with multiple cogent reasons, and provides an actionable code example with the correct project path and test framework. The response is well-structured with clear formatting. The only minor note is that it didn't explore the actual codebase to verify details, but given the self-contained nature of the prompt, that wasn't necessary. The fast 18.5s execution time with no wasted steps is appropriate.
- The agent identifies that a unit test or device test would be lighter and sufficient for testing a property setter: 5/5 — The agent clearly and immediately identifies that a unit test is the appropriate test type, stating it should be covered by a unit test in Controls.Core.UnitTests. It explicitly calls out the UI test as 'overkill' and positions the unit test as the primary coverage mechanism.
- The agent explains WHY a lighter test type is appropriate (property logic doesn't require Appium/visual UI): 5/5 — The agent provides three concrete reasons why the UI test is inappropriate: (1) it's overkill, requiring Appium, a running app, and a device/emulator for simple property behavior; (2) UI tests are slow and fragile with timing issues; (3) it's indirect — testing through multiple layers rather than directly verifying the setter logic. The agent also correctly identifies the fix as 'pure logic — no platform-specific rendering or interaction involved.'
- The recommendation is actionable, not just 'consider a unit test' — it explains what project to use or what the unit test would look like: 4.7/5 — The agent provides a complete, concrete unit test code example showing exactly what the test would look like. It also specifies the exact project location (src/Controls/tests/Core.UnitTests/). The recommendation is immediately actionable — a developer could copy the test and adapt it. It also nuances the advice by noting a UI test 'could supplement it for end-to-end confidence, but should not be the only coverage.'
With-Skill Judge (Isolated)
Overall Score: 5/5
Reasoning: The agent produced an excellent, well-structured evaluation that hits all three rubric criteria strongly. It correctly identifies the test type mismatch, provides clear reasoning about why a unit test is appropriate for a property setter, and delivers highly actionable recommendations including the target project path and complete example test code. The report also goes beyond the rubric by identifying missing edge cases, assessing flakiness risk, and evaluating assertion quality. The agent was efficient — completing the task in just 2 tool calls and 41 seconds with no errors.
- The agent identifies that a unit test or device test would be lighter and sufficient for testing a property setter: 5/5 — The agent clearly and explicitly identifies that a unit test should be used instead of a UI test. It states: 'A unit test should be used instead of a UI test. The fix is pure cross-platform property logic with no platform rendering involved.' It also provides a decision tree showing the reasoning path (Does it need visual UI? NO → Platform context? NO → Unit test). This is a direct, unambiguous identification.
- The agent explains WHY a lighter test type is appropriate (property logic doesn't require Appium/visual UI): 5/5 — The agent provides multiple clear reasons: (1) 'The fix is a property setter in cross-platform Entry.cs', (2) 'No native rendering, no platform API, no visual layout is involved', (3) 'A unit test would run in milliseconds vs. minutes, with zero flakiness risk.' It also notes the flakiness risk of the current UI test approach (no wait between tap and read). The explanation is thorough and well-reasoned.
- The recommendation is actionable, not just 'consider a unit test' — it explains what project to use or what the unit test would look like: 4.3/5 — The agent provides actionable guidance: it names the specific project ('Controls.Core.UnitTests'), and describes what the unit test would do ('create an Entry, set IsReadOnly = true, attempt to set Text, and assert it remains unchanged'). It also suggests specific edge cases to add. While it doesn't provide a full code example of the unit test, the description is concrete enough to be directly actionable. A score of 5 would require a more complete code sample, but the guidance given is clearly above 'consider a unit test' level.
With-Skill Judge (Plugin)
Overall Score: 5/5
Reasoning: The agent's evaluation is thorough, well-structured, and hits all the key points. It correctly identifies the fundamental mismatch between test type (full Appium UI test) and fix type (cross-platform property setter). Beyond just identifying the right test type, it makes the particularly insightful observation that the current test has a logical gap — checking that an already-empty Entry is empty after setting IsReadOnly proves nothing. The recommendations are specific and actionable with concrete code examples and project references. The report covers coverage, edge cases, flakiness risk, assertion quality, and convention compliance. The only minor limitation is that the agent didn't explore the actual codebase to verify project names or conventions, but its recommendations are still accurate and well-reasoned based on the information provided.
- The agent identifies that a unit test or device test would be lighter and sufficient for testing a property setter: 5/5 — The agent clearly and repeatedly identifies that the UI test is the wrong type and that a unit test would be sufficient. It states: 'A UI test is far too heavy for a cross-platform property setter change. This should be a unit test.' It also explicitly contrasts the cost: 'UI tests cost minutes in CI. This property logic is testable in milliseconds as a unit test.'
- The agent explains WHY a lighter test type is appropriate (property logic doesn't require Appium/visual UI): 5/5 — The agent provides clear reasoning: 'this is a property setter in cross-platform Entry.cs. No rendering, no platform APIs, no visual layout needed.' It also notes the indirection problem — the UI test exercises the setter through a button handler rather than directly, which is an unnecessary layer. The explanation covers both why the UI test is inappropriate and why a unit test suffices.
- The recommendation is actionable, not just 'consider a unit test' — it explains what project to use or what the unit test would look like: 5/5 — The agent provides a concrete code example of what the unit test would look like, names the specific project where it should live ('Controls.Core.UnitTests'), and even lists specific edge case tests that should be added (toggle back to false, set text before/after IsReadOnly, verify existing text preservation). This is highly actionable.
Baseline Agent Output
`
No — a UI test is the wrong primary test type here.
The fix is in cross-platform code (Entry.cs property setter), which means the logic is testable with a unit test in Controls.Core.UnitTests. Key issues:
-
Overkill: A
_IssuesUITestrequires Appium + a device/emulator, runs in themaui-pr-uitestspipeline, and takes orders of magnitude longer than a unit test. -
Indirect: The test taps buttons and reads UI elements instead of directly testing the property setter logic. It's testing the plumbing, not the fix.
-
Right approach: A unit test that creates an
Entry, setsIsReadOnly = true, and asserts the expected state — something like:
csharp [Fact] public void IsReadOnlyDisablesTextInput() { var entry = new Entry { Text = "hello" }; entry.IsReadOnly = true; // Assert the expected side effect of the setter change }
This belongs in src/Controls/tests/Core.UnitTests/ and runs in the fast maui-pr pipeline.
Bottom line: Use unit tests for cross-platform property logic. Reserve UI tests for behavior that requires a running app (visual rendering, gesture interactions, platform-specific behavior).
`
With-Skill Agent Output (Isolated)
`
PR Test Evaluation Report
Fix: Property setter in Entry.cs (cross-platform) — IsReadOnly = true disables programmatic text input
Test files evaluated: 1
Fix files: 1
Overall Verdict
❌ Tests are insufficient — wrong test type and missing edge cases.
The fix is a cross-platform property setter change — pure logic that doesn't require UI automation. A unit test should be used instead.
1. Fix Coverage — ⚠️
The test does exercise the IsReadOnly path, but indirectly through UI buttons. It's unclear whether it would catch a regression in the programmatic text-setting behavior the fix addresses.
2. Edge Cases & Gaps — ❌
Covered:
- Set IsReadOnly=true, verify text is empty
Missing:
- Toggle IsReadOnly back to
false— does text input work again? - Set
Textprogrammatically while IsReadOnly is true — is it rejected? - Set
IsReadOnlywhenTextalready has a value — is existing text preserved or cleared? - Set
IsReadOnlybefore the control is attached to a handler - Null/empty text edge cases
3. Test Type Appropriateness — ❌
Current: UI Test (Appium, _IssuesUITest)
Recommendation: Unit test — this is a property setter in cross-platform code (Entry.cs). No platform rendering, native APIs, or visual layout is involved.
`csharp
// Example unit test — much lighter, faster, less flaky
[Fact]
public void IsReadOnly_True_DisablesProgrammaticTextInput()
{
var entry = new Entry();
entry.IsReadOnly = true;
entry.Text = "should not work";
Assert.Equal("", entry.Text);
}
[Fact]
public void IsReadOnly_False_AllowsTextInput()
{
var entry = new Entry();
entry.IsReadOnly = true;
entry.IsReadOnly = false;
entry.Text = "should work";
Assert.Equal("should work", entry.Text);
}
`
Project: src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj
4. Convention Compliance — ✅
UI test conventions are followed (base class, WaitForElement, [Category]).
5. Flakiness Risk — ⚠️ Medium
UI tests are inherently flakier than unit tests. The App.Tap → check text flow has timing sensitivity.
6. Duplicate Coverage — ✅
Cannot verify without repo, but likely no duplicates for this specific scenario.
7. Platform Scope — ✅
Cross-platform fix; UI test runs on all platforms. A unit test would also cover all platforms.
8. Assertion Quality — ⚠️
Asserting text == "" is reasonable but doesn't distinguish between "text was rejected" vs "text was never set." A unit test with explicit programmatic Text = "value" would be more precise.
9. Fix-Test Alignment — ⚠️
Both target Entry, but the test exercises the UI tap path while the fix is about the property setter. A unit test would directly exercise the exact code changed.
Recommendations
- Replace the UI test with unit tests in
Controls.Core.UnitTests. The fix is a property setter — test it directly with[Fact]methods covering set-while-readonly, toggle-back, and pre-existing-text scenarios. - Add edge cases for toggling IsReadOnly off, setting text when already populated, and null text.
- Keep a UI test only if there's also a user-facing input concern (e.g., keyboard still types into the field) — but that would be a separate scenario from the programmatic fix.
With-Skill Agent Output (Plugin)
`
PR Test Evaluation Report
Test files evaluated: 1
Fix files: 1 (Entry.cs — cross-platform property setter)
Overall Verdict
The fix is a cross-platform property setter change — pure logic testable with a unit test. A UI test is the heaviest option and overkill here.
1. Fix Coverage — ⚠️
The test sets IsReadOnly = true via a button tap and checks text is empty. It exercises the code path indirectly through UI, but doesn't verify the core behavior: that programmatic text input is blocked when IsReadOnly = true.
2. Edge Cases & Gaps — ❌
Covered:
- Setting
IsReadOnly = trueon an empty Entry
Missing:
- Setting
IsReadOnly = truewhen Entry already has text — is existing text preserved? - Toggling
IsReadOnlyback tofalse— does input work again? - Setting text programmatically while
IsReadOnly = true— is it rejected? - Default state —
IsReadOnly = falseallows input
3. Test Type Appropriateness — ❌
Current: UI Test (Appium, _IssuesUITest)
Recommendation: Unit test — this is a property setter in cross-platform Entry.cs. No native rendering, no visual layout, no user interaction flow needed. A unit test can set IsReadOnly = true, attempt to set Text, and assert the result directly.
csharp [Fact] public void IsReadOnlyPreventsTextSet() { var entry = new Entry(); entry.IsReadOnly = true; entry.Text = "should be blocked"; Assert.Equal("", entry.Text); }
4. Convention Compliance — ⚠️
- Missing
[Category]attribute check (only on method, needs to be verified it's not duplicated on class) - No
WaitForElementbeforeApp.Tap("SetReadOnlyButton")— only waits forTestEntry
5. Flakiness Risk — ⚠️ Medium
- Missing
WaitForElement("SetReadOnlyButton")before tap - UI tests are inherently flakier than unit tests for property logic
6. Duplicate Coverage — ✅ No duplicates noted
7. Platform Scope — ✅
Cross-platform fix, UI test runs on all platforms.
8. Assertion Quality — ⚠️
Asserts text is empty string — but the Entry starts empty. The test would pass even if the fix didn't work. Should set text before or after setting ReadOnly and verify it's blocked.
9. Fix-Test Alignment — ⚠️
Both target Entry, but the test doesn't directly exercise "programmatic text input is disabled" — it just checks an already-empty Entry is still empty.
Recommendations
- Replace with a unit test in
Controls.Core.UnitTests. The fix is a cross-platform property setter — no platform context needed. - Fix the assertion gap — the test currently passes even without the fix (empty Entry stays empty). Set text after setting
IsReadOnly = trueand assert it's rejected. - Add edge cases: toggle back to
false, pre-existing text preservation, default behavior.
📊 evaluate-pr-tests / weak-assertion-detection-meaningless-test-assertions
Judge Report: Weak assertion detection - meaningless test assertions
Baseline Judge
Overall Score: 5/5
Reasoning: The agent delivered an excellent, well-structured analysis without needing any tool calls — which is appropriate since this was a pure code review question requiring no codebase exploration. It correctly identified both assertions as inadequate, explained why with precise technical reasoning (tautology, framework-level redundancy), provided concrete replacement code examples, and presented a clear summary table. The response is thorough, actionable, and well-organized.
- The agent correctly identifies Assert.That(true) as a meaningless assertion that proves nothing about the fix: 5/5 — The agent explicitly calls out
Assert.That(true)as a 'tautology' that 'can never fail,' and further explains that the supposed 'no crash' safety net is already inherently provided byWaitForElement. This is a precise and thorough identification of the problem. - The agent identifies that checking Is.Not.Null on a UI element is too vague to catch actual regressions: 5/5 — The agent correctly identifies that
App.FindElementthrows if the element isn't found, making theIs.Not.Nullcheck redundant. It further explains that what's missing is an assertion on the label's content rather than its mere existence. This is an accurate and insightful analysis. - The agent provides concrete examples of what specific assertions SHOULD look like to catch the regression: 5/5 — The agent provides two full rewritten test methods with concrete assertions (e.g.,
Assert.That(status, Is.EqualTo("No selection"))andAssert.That(text, Is.EqualTo("Loaded 10 items"))). While the exact expected strings are speculative (since we don't know the actual app), they serve as clear, illustrative examples of the pattern: assert on observable outcomes. The agent also mentions alternative approaches like checking SelectedItem count or visual state changes. - The overall verdict reflects that the assertions are insufficient, not just a minor concern: 5/5 — The agent opens with a clear, unambiguous verdict: 'both assertions are effectively no-ops and won't catch regressions.' The closing principle reinforces this: 'the tests pass even when the feature is broken.' This is appropriately strong and not hedged as a minor concern.
With-Skill Judge (Isolated)
Overall Score: 5/5
Reasoning: The agent delivered a concise, accurate, and thorough review without needing any tool calls (none were necessary for this code-review question). It correctly identified all key issues: the tautological assertion, the redundant null check, and the absence of meaningful state verification. It provided concrete, idiomatic code suggestions and framed the overall verdict appropriately. The response is well-structured, actionable, and demonstrates strong understanding of UI test best practices. The efficiency is also notable — zero tool calls were needed and the response was delivered quickly.
- The agent correctly identifies Assert.That(true) as a meaningless assertion that proves nothing about the fix: 5/5 — The agent clearly and accurately identifies
Assert.That(true)as 'a no-op — it always passes' and explains it 'only detects crashes, not whether selection was actually cleared.' This is precise and correct. - The agent identifies that checking Is.Not.Null on a UI element is too vague to catch actual regressions: 5/5 — The agent correctly explains that
Is.Not.Null'only proves the label exists in the visual tree — it says nothing about whether the CollectionView loaded data.' This is an accurate and well-articulated critique. - The agent provides concrete examples of what specific assertions SHOULD look like to catch the regression: 4.7/5 — The agent provides concrete code examples for both tests: asserting on label text for selection state (
Is.EqualTo("Selection: None")) and asserting on meaningful content for load state (Does.Contain("Loaded")). These are reasonable illustrative suggestions. They lose a point because the examples are necessarily somewhat speculative (the agent doesn't know the actual UI structure), but they clearly communicate the principle of asserting on observable behavioral state rather than mere existence. - The overall verdict reflects that the assertions are insufficient, not just a minor concern: 5/5 — The agent's verdict is unequivocal: 'Both are smoke tests, not regression tests. They'll pass even if the feature is completely broken (as long as it doesn't crash).' The summary table reinforces this with ❌ marks on all functional criteria. The opening line — 'both assertions are too weak to catch regressions' — sets a clear tone. This is appropriately strong.
With-Skill Judge (Plugin)
Overall Score: 5/5
Reasoning: The agent delivered an excellent, well-structured review that correctly identifies all the problems with both test assertions, explains why they are inadequate, provides concrete improvement suggestions with code, and frames the overall verdict with appropriate severity. The response is concise, actionable, and demonstrates strong understanding of UI testing best practices. No tools were needed for this code review task, and the agent correctly proceeded without them.
- The agent correctly identifies Assert.That(true) as a meaningless assertion that proves nothing about the fix: 5/5 — The agent clearly and immediately identifies
Assert.That(true)as 'a no-op — it always passes' and notes thatWaitForElementalready implicitly checks for no crash, making the explicit assert entirely redundant. This is precise and correct. - The agent identifies that checking Is.Not.Null on a UI element is too vague to catch actual regressions: 5/5 — The agent correctly explains that
App.FindElementtypically throws if the element doesn't exist (makingIs.Not.Nullredundant), and more importantly that the label could display an error message and the test would still pass. This demonstrates a clear understanding of why the assertion is too weak. - The agent provides concrete examples of what specific assertions SHOULD look like to catch the regression: 4.7/5 — The agent provides two concrete code examples: checking that
StatusLabeltext equals 'Selection: None' for test 1, and checking that the text contains 'Loaded' for test 2. These are reasonable and illustrative suggestions. The slight limitation is that without examining the actual codebase, the specific text values are hypothetical — but the agent is upfront about the pattern and the examples clearly communicate the right approach. TheGetText()API may not be the exact correct API for the framework being used, but the intent is clear. - The overall verdict reflects that the assertions are insufficient, not just a minor concern: 5/5 — The agent opens with a strong, unambiguous verdict: 'both tests have effectively no meaningful assertions.' It closes with a powerful rule of thumb: 'if deleting the code under test wouldn't fail the test, the assertion isn't catching anything.' This leaves no doubt that the assertions are fundamentally inadequate, not just slightly weak.
Baseline Agent Output
`
No, both assertions are weak and unlikely to catch real regressions.
Test 1 — SelectionClearsOnNull
Assert.That(true)is a tautology — it always passes. It tests nothing.- The "no crash" goal is already covered by
WaitForElement; if the app crashes, that call times out and fails. - The real regression question — did the selection actually clear? — is never verified. You need something like:
csharp App.Tap("ClearSelectionButton"); var label = App.WaitForElement("StatusLabel"); Assert.That(label.GetText(), Is.EqualTo("No selection"));
Test 2 — CollectionViewLoads
Assert.That(elem, Is.Not.Null)is redundant becauseApp.FindElementalready throws/times out if the element doesn't exist. The test would fail before reaching the assert.- It never checks the label's content, so a regression that loads the wrong data or shows an error message would pass silently:
csharp var elem = App.WaitForElement("StatusLabel"); Assert.That(elem.GetText(), Is.EqualTo("Loaded 10 items"));
Summary of problems:
| Issue | Impact |
|---|---|
Assert.That(true) |
Dead assertion, never fails |
Null-check on FindElement |
Redundant, framework already throws |
| No state verification after action | Selection bugs slip through undetected |
| No text/content checks | Wrong-data regressions pass silently |
Both tests should assert on observable state changes (label text, selection count, item visibility) rather than mere existence or tautologies.
`
With-Skill Agent Output (Isolated)
`
No, both tests have weak assertions that would pass even when the feature is broken.
Test 1 — SelectionClearsOnNull
Assert.That(true)is a tautology — it always passes. The "no crash" check is already covered byApp.WaitForElementthrowing on crash.- Missing: Verify the selection was actually cleared (e.g., check a label bound to
SelectedItem, or assertSelectedItems.Count == 0).
Test 2 — CollectionViewLoads
App.FindElementalready throws if the element isn't found, makingIs.Not.Nullredundant.- Missing: Assert on the element's content (e.g.,
StatusLabel.Text == "Loaded"or item count).
Suggested improvements:
`csharp
// Test 1: Actually verify selection was cleared
App.Tap("ClearSelectionButton");
var status = App.WaitForElement("SelectionStatusLabel");
Assert.That(status.GetText(), Is.EqualTo("None"));
// Test 2: Verify meaningful state, not just existence
var elem = App.WaitForElement("StatusLabel");
Assert.That(elem.GetText(), Does.Contain("Loaded"));
`
Rule of thumb: If deleting the line under test wouldn't cause the test to fail, the assertion isn't catching anything.
`
With-Skill Agent Output (Plugin)
`
No, both assertions are effectively no-ops and won't catch regressions.
Test 1 — SelectionClearsOnNull
Assert.That(true)can never fail. It asserts nothing.WaitForElementalready verifies no crash, making the assert redundant.- Missing: Verify selection was actually cleared — e.g., check a label showing selected count, or query
SelectedItems.
csharp App.Tap("ClearSelectionButton"); var label = App.WaitForElement("SelectionCountLabel"); Assert.That(label.GetText(), Is.EqualTo("0"));
Test 2 — CollectionViewLoads
App.FindElementalready throws if the element isn't found, soIs.Not.Nullis redundant.- Missing: Assert on the label's content — the element existing tells you nothing about whether the CollectionView loaded correctly.
csharp var elem = App.WaitForElement("StatusLabel"); Assert.That(elem.GetText(), Is.EqualTo("Loaded"));
Summary: Both tests will pass even if the underlying feature is completely broken. Good UI tests should assert on observable state changes, not just element existence.
`
|
@JanKrivanek why the Skill Validation run for this PR? |
🔍 Multi-Model Code Review — PR #35068 (Re-Review)Bridge DI-registered Essentials implementations to static facades CI Status: ⏳ PendingmacOS Pack passed ✅. Windows/macOS builds and Helix unit tests still running. Previous Prior Review CommentsPrevious review from this reviewer addressed. No other substantive review comments. Previous Findings — Status After Fixes
New Findings🟡 MODERATE —
|
| Finding | Flagged by | Reason for discard |
|---|---|---|
MauiContacts alias position is unconventional among using directives |
1/3 | Style/formatting — explicitly out of scope |
Test Coverage Assessment
The PR now includes 12 unit tests covering:
- ✅ SetDefault pattern types (Preferences, Battery, Contacts)
- ✅ SetCurrent pattern types (AppInfo, Connectivity)
- ✅ Geocoding naming quirk (SetCurrent + defaultImplementation field)
- ✅ No-registration leaves backing fields null (both patterns)
- ✅ Multiple simultaneous DI registrations
- ✅ ConfigureEssentials compatibility
- ✅ Transient DI lifetime (with documentation)
- ✅ Precedence/coexistence with ConfigureEssentials
Remaining gaps: No test for platform-specific types (ActivityStateManager, WindowStateManager) since unit tests run on host TFM only. Acceptable — these are compile-time guards.
All 39 SetDefault/SetCurrent methods from Essentials are covered in the bridge list ✅.
Recommended Action: ⚠️ Request Changes
Required before merge:
- Change
#if WINDOWS || __IOS__to#if WINDOWS || __IOS__ || MACCATALYSTforIWindowStateManagerbridge — follows the codebase's dominant pattern and prevents a silent macCatalyst gap
Recommended but non-blocking:
2. Rename DIRegistration_TakesPrecedenceOverConfigureEssentials to accurately reflect what it tests (coexistence, not precedence)
The overall design is sound — all previous critical/moderate issues have been addressed. One platform guard fix and this PR is ready to merge.
1193288 to
3d0d484
Compare
3d0d484 to
0141926
Compare
|
/review |
|
✅ Expert Code Review completed successfully! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert Code Review — PR #35068
Methodology: 3 independent reviewers with adversarial consensus. Findings flagged by 2+ reviewers are included; 1/3 disputed findings were sent to the other 2 reviewers for follow-up evaluation.
Summary
The core DI bridge mechanism (BridgeEssentialsFromDI + BridgeIfRegistered<T>) is clean, correct, and well-tested. The approach avoids new public API surface and aligns well with the MAUI hosting model. Tests cover the key scenarios including both SetDefault/SetCurrent patterns, no-registration passthrough, and the Geocoding naming quirk.
Two moderate findings and three minor suggestions were identified through consensus.
Findings by Severity
🟡 MODERATE
| # | Finding | Consensus | File |
|---|---|---|---|
| 1 | MainThreadBridgeInitializer guard removal — Now runs on all platforms including native ones where s_mainThreadImplementation is never read. Dead work + static retention of IDispatcher-capturing lambdas. Unrelated to the DI bridge feature. |
2/3 reviewers | EssentialsMauiAppBuilderExtensions.cs:102 |
| 2 | IAppActions bridge lacks IPlatformAppActions guard — Platform lifecycle hooks cast AppActions.Current to IPlatformAppActions via AsPlatform(). A DI-registered IAppActions that doesn't implement IPlatformAppActions would throw at runtime on Android/iOS. |
3/3 reviewers (after follow-up; 1 reviewer downgraded from Error to Warning) | EssentialsMauiAppBuilderExtensions.cs:244 |
🟢 MINOR
| # | Finding | Consensus | File |
|---|---|---|---|
| 3 | Preprocessor symbol inconsistency — MACCATALYST vs __MACCATALYST__ in the same file |
2/3 reviewers | EssentialsMauiAppBuilderExtensions.cs:238 |
| 4 | Silent lifetime promotion — Transient/Scoped DI registrations stored in static fields become de facto singletons | 2/3 reviewers | EssentialsMauiAppBuilderExtensions.cs:259 |
| 5 | Duplicate test methods — DIBridge_WorksWithConfigureEssentials and DIRegistration_WorksAlongsideConfigureEssentials are identical |
2/3 reviewers | EssentialsDIBridgeTests.cs:173,242 |
Discarded (single reviewer, not confirmed)
- Unconditional
AppActions.OnAppActionsubscription — 1/3; both follow-ups confirmed the handler is a no-op when noAppActionHandlersare configured (null-conditional?.Invoke()).
CI & Test Coverage
- Tests: 10 new unit tests covering SetDefault types, SetCurrent types, Geocoding quirk, no-registration passthrough, multiple registrations, ConfigureEssentials compatibility, transient lifetime, and Contacts namespace collision. Good coverage of the bridge mechanism.
- Gap: No test verifies the
IAppActions/IPlatformAppActionsscenario (Finding #2). - CI status: Mergeable state shows "unstable" — worth verifying CI results before merge.
Generated by Expert Code Review for issue #35068 · ● 24.8M
| }); | ||
|
|
||
| #if !(ANDROID || __IOS__ || __MACCATALYST__ || WINDOWS || TIZEN) | ||
| builder.Services.TryAddEnumerable(ServiceDescriptor.Transient<IMauiInitializeService, MainThreadBridgeInitializer>()); |
There was a problem hiding this comment.
🟡 MODERATE — MainThreadBridgeInitializer now runs on all platforms (2/3 reviewers)
The #if !(ANDROID || __IOS__ || __MACCATALYST__ || WINDOWS || TIZEN) guard was removed from both this registration and the class definition. MainThreadBridgeInitializer.Initialize() now calls MainThread.SetCustomImplementation() on every platform, but on native platforms (Android/iOS/Windows/Tizen) the s_mainThreadImplementation field it writes is never read — PlatformIsMainThread and PlatformBeginInvokeOnMainThread are direct partial-class overrides.
This means: (1) it's dead work on native platforms, (2) the lambdas capturing IDispatcher are retained in a static field for the app lifetime with no clear disposal path, and (3) the class's own XML doc says it's for "custom platform backends / external TFMs where no native MainThread implementation exists."
Recommendation: Either restore the #if guard (this change is unrelated to the DI bridge feature) or update the XML doc and add a comment explaining why it now runs unconditionally.
| BridgeIfRegistered<IAppleSignInAuthenticator>(services, AppleSignInAuthenticator.SetDefault); | ||
|
|
||
| // SetCurrent pattern types | ||
| BridgeIfRegistered<IAppActions>(services, AppActions.SetCurrent); |
There was a problem hiding this comment.
🟡 MODERATE — DI-bridged IAppActions may lack IPlatformAppActions (3/3 reviewers after follow-up)
Platform lifecycle hooks (registered by UseEssentials()) call AppActions.Current.OnNewIntent(intent) etc., which internally casts to IPlatformAppActions via AsPlatform() and throws PlatformNotSupportedException if the cast fails. Before this PR, only internal code could set AppActions.Current; now any DI-registered IAppActions is automatically promoted.
If a user registers a custom IAppActions that doesn't also implement IPlatformAppActions, platform lifecycle events (Android OnNewIntent, iOS PerformActionForShortcutItem) will throw at runtime.
Recommendation: Add an XML doc remark on BridgeEssentialsFromDI noting that DI-registered IAppActions implementations must also implement IPlatformAppActions on supported platforms. Alternatively, add a runtime guard: only bridge IAppActions if the implementation also implements IPlatformAppActions.
| } | ||
|
|
||
| [Fact] | ||
| public void DIRegistration_WorksAlongsideConfigureEssentials() |
There was a problem hiding this comment.
🟢 MINOR — Duplicate test (2/3 reviewers)
DIRegistration_WorksAlongsideConfigureEssentials (here) and DIBridge_WorksWithConfigureEssentials (line 173) are functionally identical — both register IPreferences, call ConfigureEssentials(), build, and assert Preferences.Default. They cover the same scenario with slightly different comments.
Recommendation: Remove one, or differentiate them (e.g., have one pass a real delegate like e => e.UseVersionTracking()).
| /// promoting it to singleton scope regardless of its DI registration lifetime. Services bridged | ||
| /// here should be registered as Singleton for correct behavior. | ||
| /// </summary> | ||
| static void BridgeIfRegistered<T>(IServiceProvider services, Action<T?> setter) where T : class |
There was a problem hiding this comment.
🟢 MINOR — Non-singleton DI registrations silently promoted to singleton (2/3 reviewers)
BridgeIfRegistered<T> resolves one instance via GetService<T>() and stores it in a static field forever, effectively promoting Transient/Scoped registrations to Singleton scope. The XML doc correctly notes this, and TransientDIRegistration_StillBridged tests it — but users won't read the XML doc before registering AddTransient<IBattery, MyBattery>().
Recommendation (nice-to-have): Consider logging a diagnostic warning when the resolved service's registration lifetime is not Singleton, to help developers catch unintended lifetime promotion.
| BridgeIfRegistered<IVersionTracking>(services, VersionTracking.SetDefault); | ||
| BridgeIfRegistered<IVibration>(services, Vibration.SetDefault); | ||
| BridgeIfRegistered<IWebAuthenticator>(services, WebAuthenticator.SetDefault); | ||
| #if WINDOWS || __IOS__ || MACCATALYST |
There was a problem hiding this comment.
🟢 MINOR — Preprocessor symbol inconsistency (2/3 reviewers)
This uses MACCATALYST (no underscores) while every other Apple-platform guard in this file — including the guard just removed — uses __MACCATALYST__ (double underscores). Both symbols are valid in .NET MAUI, but the inconsistency within the same file is confusing.
Recommendation: Change to #if WINDOWS || __IOS__ || __MACCATALYST__ for consistency.
12c73fc to
dadabc9
Compare
) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue Details: Horizontalspacing / Verticalspacing is not not applied to the first column in GridItemLayout using CollectionView on Android platform. ### Root Cause: The grid spacing was not being distributed symmetrically across the active layout implementations, so edge items did not fully participate when spacing changed at runtime. ### Description of Change: - On Android, the fix in MauiRecyclerView.cs changes how RecyclerView padding is handled for GridItemsLayout. Android was already using SpacingItemDecoration, which applies half-spacing on all four sides of each item. Previously, negative RecyclerView padding canceled that spacing at the control edges. The branch keeps that negative-padding behavior for non-grid layouts, but disables it for GridItemsLayout, allowing the grid’s half-spacing to remain visible at the outer perimeter. This makes the first row and first column visually respond when spacing changes, but it also changes the grid behavior from spacing only between items to spacing around the outside edges as well. **Tested the behavior in the following platforms:** - [x] Android - [x] Windows - [ ] iOS - [ ] Mac ### Reference: N/A ### Issues Fixed: Fixes #34257 ### Screenshots | Before | After | |---------|--------| | <Video src="https://github.com/user-attachments/assets/578dda69-1d60-474c-a6d8-23b3f9d29a50" Width="300" Height="600"> | <Video src="https://github.com/user-attachments/assets/7f3826e6-5922-4b6f-a6b9-de581b7db6c3" Width="300" Height="600"> |
…32491) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Summary Fixes a critical bug where `HybridWebView.InvokeJavaScriptAsync` would timeout when passing JSON strings as parameters. The issue was caused by improper JavaScript string escaping. The fix extracts business logic into a new `HybridWebViewHelper` class and uses `WebViewHelper.EscapeJsString` for proper escaping. **Fixes**: #32438 --- ## The Problem Issue #32438 reported that passing JSON strings to `InvokeJavaScriptAsync` caused timeouts on Windows (required base64 encoding as workaround), while Android worked correctly. ### Root Cause The old `EvaluateJavaScriptAsync` implementation used naive string concatenation: ```csharp // OLD - Breaks when script contains quotes script = "try{eval('" + script + "')}catch(e){'null'};"; ``` When the script contained JSON with quotes, this produced invalid JavaScript: ```javascript try{eval('window.HybridWebView.__InvokeJavaScript(1, 'method', ["{\"userId\":\"value\"}"])')}catch(e){'null'}; ↑ BROKEN - conflicting quotes ``` Result: JavaScript execution failed, causing `InvokeJavaScriptAsync` to timeout. --- ## The Solution ### Core Fix New implementation properly escapes JavaScript strings: ```csharp // NEW - Uses proper escaping var escapedScript = WebViewHelper.EscapeJsString(script); var wrappedScript = $$""" (function() { try { let result = eval('{{escapedScript}}'); // ← Properly escaped return JSON.stringify({ IsError: false, Result: JSON.stringify(result) }); } catch (error) { // ... error handling } })() """; ``` ### Refactoring Extracted ~360 lines from `HybridWebViewHandler` into new `HybridWebViewHelper` class: - `ProcessEvaluateJavaScriptAsync` - Script wrapping with proper escaping - `ProcessInvokeJavaScriptAsync` - JavaScript call building - `ProcessInvokeDotNetAsync` - .NET method invocation from JS - `ProcessRawMessage` - Message routing Handler reduced from ~600 to 244 lines. --- ## Changes **New Files:** - ✨ `HybridWebViewHelper.cs` (470 lines) - Centralized business logic **Modified Files:** - 🔧 `HybridWebViewHandler.cs` - Delegates to helper - 🔧 `HybridWebViewHandler.Standard.cs` - Updated message processing - 🔧 `HybridWebViewHandler.Tizen.cs` - Updated message processing **Tests:** - 🔧 `HybridWebViewTestsBase.cs` - Added 15-second timeout - ✨ Added 5 new tests for JSON parameter scenarios - ✨ Added 3 enhanced quote-handling tests - 🔧 Renamed 20+ tests for clarity - 📝 Corrected error messages to use `InvokeJavaScriptAsync` **Test HTML:** - Added `EchoJsonParameter`, `ParseAndStringifyJson`, `ConcatenateJsonStrings`, `DecodeBase64AndEcho`, `CountJsonArrayItems` --- ## Testing **Platforms**: iOS, Android, Windows, MacCatalyst **Scenarios**: - JSON with quotes and special characters - Complex nested JSON - Multiple JSON parameters - Large arrays (100 items) - Base64 workaround (backward compatibility) --- ## Breaking Changes **None** - Fully backward compatible: - ✅ Existing code works unchanged - ✅ Base64 workaround still works - ✅ No public API changes ### Migration Note If using base64 workaround, you can now simplify: ```csharp // OLD workaround (still works): var json = JsonSerializer.Serialize(obj); var base64 = Convert.ToBase64String(Encoding.UTF8.GetBytes(json)); await hybridWebView.InvokeJavaScriptAsync<string>("fn", ..., [base64], ...); // NEW (now works directly): var json = JsonSerializer.Serialize(obj); await hybridWebView.InvokeJavaScriptAsync<string>("fn", ..., [json], ...); ``` --- ## Checklist - [x] Code follows .NET MAUI conventions - [x] 8 new/enhanced tests added - [x] 20+ tests renamed for clarity - [x] No breaking changes - [x] Backward compatible - [x] Error messages corrected - [x] Formatted with `dotnet format` - [x] XML documentation complete - [x] Tested on multiple platforms --------- Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: mattleibow <1096616+mattleibow@users.noreply.github.com>
… on collection update (#31275) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause **Windows** Position not updating on item add: CarouselView stayed at the old position after an item was added, leaving current/previous positions unsynced. Cascading events: With `ItemsUpdatingScrollMode.KeepItemsInView`, programmatic smooth scrolls triggered multiple ViewChanged calls, causing `PositionChanged` to fire repeatedly with intermediate values. **Android** Programmatic smooth scrolls produced the same cascading `PositionChanged` events as on Windows. ### Description of Change **Windows** Position Update: On item add, `ItemsView.Position` is explicitly set based on `ItemsUpdatingScrollMode`, keeping current and previous positions in sync. Prevent Cascading Events: Added `_isInternalPositionUpdate`. For collection changes, animations are disabled (animate = false) so scrolling jumps directly, firing `PositionChanged` only once. **Android** Reused the `_isInternalPositionUpdate` logic. Disabled animations during collection changes, ensuring a single clean position update without duplicate events. ### Issues Fixed Fixes #29529 Tested the behaviour in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Screenshots | Before Issue Fix | After Issue Fix | |------------------|-----------------| | <img width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/d66f0352-a91f-4b85-bb9f-e0e54e55aa5f" /> | <img width="350" alt="withfix" src="https://github.com/user-attachments/assets/d120f268-6954-498d-aab0-42bc3745e296" /> | ---------
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details On iOS/MacCatalyst 26+, when the app theme changes (light ↔ dark), the Switch's ThumbColor resets to white instead of keeping the custom color. ### Root Cause On iOS/MacCatalyst 26+, UIKit resets the UISwitch's ThumbTintColor to its default value during theme transitions. ### Description of Change Register for trait collection changes to detect theme switches, then re-apply the custom ThumbColor after UIKit completes its styling. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #33783 Fixes #33767 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/aa595096-4f75-4d7e-b31e-f1f0acc24208" >| <video src="https://github.com/user-attachments/assets/386f9e6e-6df7-40c5-a3e7-59a0bde31df6">|
<!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details WebView does not scroll when placed inside a ScrollView. The parent ScrollView intercepts vertical touch gestures, preventing the WebView from scrolling its internal content. ### Root Cause The parent ScrollView was intercepting all touch events without checking if the child WebView needed to scroll. This prevented the WebView from receiving touch events and handling its own scrolling. ### Description of Change Added touch event handling to prevent parent interception when WebView scrolls. When touch begins or continues, WebView requests exclusive control from parent. When touch ends or cancels, control returns to parent. This enables WebView scrolling while preserving normal parent-child interaction. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #32971 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/12b3ca6e-582d-4a50-9ea1-f49027f2d907" >| <video src="https://github.com/user-attachments/assets/2fbfd03c-4432-49e9-8b2d-6e7643f57487">|
… to null (#34741) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! <!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Issue Details On iOS and macCatalyst, setting BackgroundColor = null on an Entry or Editor at runtime does not restore the native default appearance. Once a custom color is applied, it persists even after the property is cleared. ### Root Cause The shared iOS background update logic only resets BackgroundColor = null for LayoutView subclasses — all other UIView types silently return. MauiTextField (Entry) and MauiTextView (Editor) are not LayoutView subclasses, so the reset is skipped. ### Description of Change Platform-specific MapBackground methods were added to EntryHandler.iOS.cs and EditorHandler.iOS.cs. When Background.IsNullOrEmpty(), they now explicitly set platformView.BackgroundColor = null to restore native appearance. Validated the behavior in the following platforms - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Issues Fixed Fixes #34611 ### Output ScreenShot |Before|After| |--|--| | <video src="https://github.com/user-attachments/assets/5ca30c6d-c069-4c04-989b-4dae36584cb4" >| <video src="https://github.com/user-attachments/assets/ee9e2a2e-c210-47cc-9f85-2526780d398b">| --------- Co-authored-by: Jakub Florkowski <42434498+kubaflo@users.noreply.github.com>
…t placed child to the Border control in iOS/ Mac platform (#33330) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Issue Details: Stacklayout is not rendered when clip is applied and StackLayout placed child to the Border control in iOS/ Mac platform. ### Root Cause: When the clip is applied to the StackLayout, its ContainerView (WrapperView) is being inserted at index 0 to its parent control Border, which places it below Border's background layer in the z-order. As a result, the stacklayout is not visible in the view in iOS and Mac platform. ### Description of Change: The wrapper view is brought to the front of its parent’s subview stack so it renders above the Border background in the Z order. **Tested the behavior in the following platforms.** - [x] Android - [x] Windows - [x] iOS - [x] Mac ### Reference: N/A ### Issues Fixed: Fixes #33241 ### Screenshots | Before | After | |---------|--------| | <img width="369" height="606" alt="image" src="https://github.com/user-attachments/assets/0be8bc27-5de4-41ad-a41f-92581513ac55" /> | <img width="369" height="606" alt="image" src="https://github.com/user-attachments/assets/6a3590f6-2763-473a-aa91-ee1113e48ec3" /> |
…cription is set (#33979) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ### Root Cause WinUI `TextBlock` (used by `Label`) automatically exposes its `Text` to UI Automation. `ContentPanel` uses the default `FrameworkElementAutomationPeer`, which exposes both the parent’s `AutomationProperties.Name` and all child elements. Unlike Android (`NoHideDescendants`) or iOS (`AccessibilityElementsHidden`), Windows has no single property to hide descendants while keeping the parent accessible. As a result, both Tab navigation and Browse mode announced duplicate content. ### Description of Change Implemented a custom `ContentPanelAutomationPeer` that overrides three core UI Automation methods (GetAutomationControlTypeCore , `GetLocalizedControlTypeCore`, `GetChildrenCore`) to conditionally modify behavior when Description is present. When a Description exists, the control is exposed as `AutomationControlType.Text` (enables browse mode navigation; alternatives like Custom announce "custom" suffix, Group causes browse mode to skip the element), the "text" announcement suffix is suppressed via empty `GetLocalizedControlTypeCore()` return, and child elements are hidden by returning null from `GetChildrenCore()` to prevent duplication. When no Description is present, default behavior is preserved with `AutomationControlType.Custom` and children remain accessible. The `HasDescription` helper property centralizes the non-empty Description check across all three override methods, ensuring consistent conditional logic following the MAUI `AutomationPeer patterns. ### Issues Fixed Fixes #33373 ### Platforms Tested - [ ] iOS - [x] Android - [x] Windows - [x] Mac ### Screenshots | Before Fix | After Fix | |------------|-----------| | <video width="350" alt="withoutfix" src="https://github.com/user-attachments/assets/c5f8f114-fbeb-42d1-8601-e75dad57a1a7" /> | <video width="350" alt="withfix" src="https://github.com/user-attachments/assets/d2405df5-64f3-4cd9-8b38-40911ce4fbd6" /> | ---------
<!-- !!!!!!! MAIN IS THE ONLY ACTIVE BRANCH. MAKE SURE THIS PR IS TARGETING MAIN. !!!!!!! --> ### Description of Change <img width="870" height="73" alt="image" src="https://github.com/user-attachments/assets/e27cb50d-db93-431b-831c-24f7faedf9b1" /> Invalidation propagation can be quite consuming especially when switching binding context and multiple properties change (i.e. having multiple labels inside a layout, all of them changing the text). Ti PR avoids useless propagations (same behavior `requestLayout` intrinsically has on Android). # Conflicts: # src/Core/src/Platform/iOS/MauiView.cs
This comment has been minimized.
This comment has been minimized.
1f8dd5f to
84345dc
Compare
16dc4f5 to
7c04ed7
Compare
…ess review Brings the Essentials DI bridge branch up to date with inflight/current. Conflict resolution: - MainThread bridge defers to the already-merged #35070 implementation (non-throwing dispatcher.Dispatch); the branch's throw-on-failure variant and its DispatchReturnsFalse test were dropped to avoid diverging from shipped behavior. Kept the MainThreadStaticState test-serialization collection. - Removed a duplicate MainThreadBridgeInitializer registration introduced by the merge (the feature registers it early, by design). Dropped unrelated drift the stale branch carried (other in-flight PRs that inflight/current no longer includes): HybridWebView refactor, Issue34671, MauiRecyclerView, Label/ScrollViewer extensions, CollectionView scrolling test. The PR now contains only the Essentials DI bridge + its tests. Expert-review fixes applied to the bridge: - IWebAuthenticator: guard with IPlatformWebAuthenticatorCallback on Android/iOS/MacCatalyst before replacing the static facade (mirrors IAppActions), preventing PlatformNotSupportedException from platform callback hooks. - IActivityStateManager: no longer bridged (Android-only; its default is already initialized with lifecycle callbacks before the bridge runs, and custom backends never hit the Android path). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
@Redth I brought this up to date with Conflict resolution / cleanup
Expert-review [major] findings addressed
The previously-flagged |
|
/review -b feature/enhanced-reviewer |
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
Expert Review — 1 findings
See inline comments for details.
| static void BridgeIfRegistered<T>(IServiceProvider services, Action<T?> setter) where T : class | ||
| { | ||
| var impl = services.GetService<T>(); | ||
| if (impl is not null) |
There was a problem hiding this comment.
[major] Logic and Correctness — BridgeIfRegistered only assigns a facade when DI resolves a service, so static Essentials state from an earlier MauiApp.Build() is never cleared. Concrete scenario: build app A with IPreferences registered, which sets Preferences.Default to A's backend; then dispose it and build app B without IPreferences. Because this branch does not call SetDefault(null), Preferences.Default still returns app A's stale backend instead of falling back to the platform default. The added tests reset static fields before each test and therefore do not cover repeated-build lifecycle behavior. Please clear/reset bridged facades when a service is not registered.
MauiBot
added
s/agent-fix-win
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@Redth — new AI review results are available based on this last commit:
b707a1c. To request a fresh review after new comments or commits, comment/review rerun.
Review Sessions — click to expand
Gate — Test Before & After Fix
Gate Result: ❌ FAILED
Platform: ANDROID
⚠️ verify-tests-fail.ps1exited before writing a verification report. Diagnostics below.
Exit code: 1
Artifacts written before exit:
verification-log.txt(37.2 KB)
Gate output log (last 60 lines)
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Platform/iOS/Extensions/FormattedStringExtensions.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PromptArguments.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/net-ios/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/net-tizen/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/net-windows/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/net/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/PublicAPI/netstandard/PublicAPI.Unshipped.txt exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/RadioButton/RadioButton.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/ResourcesExtensions.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/ScrollView/ScrollView.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Setter.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Shell/Shell.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Shell/ShellNavigationManager.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Shell/ShellSection.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/ShellToolbar.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/SwipeView/SwipeItem.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/SwipeView/SwipeItemView.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/SwipeView/SwipeItems.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/SwipeView/SwipeView.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Toolbar/Toolbar.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/VisualElement/VisualElement.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/VisualStateManager.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/WebView/WebViewSource.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Window/Window.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Core/Window/Window.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/SourceGen/KnownMarkups.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/SourceGen/NodeSGExtensions.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Xaml/MarkupExtensions/BindingExtension.cs exists
[2026-06-12 16:52:53] ✓ src/Controls/src/Xaml/XamlServiceProvider.cs exists
[2026-06-12 16:52:53] ✓ src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformDrawableStyle.java exists
[2026-06-12 16:52:53] ✓ src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformWrapperView.java exists
[2026-06-12 16:52:53] ✓ src/Core/maps/src/Handlers/Map/MapElementPlatformOptions.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Core/maps/src/Handlers/Map/MapHandler.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Core/maps/src/Handlers/Map/MapHandler.iOS.cs exists
[2026-06-12 16:52:53] ✓ src/Core/maps/src/Platform/iOS/MauiMKMapView.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Animations/AnimationManager.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Core/IMauiRecyclerView.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Core/ISafeAreaView2.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Diagnostics/DiagnosticsManager.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Diagnostics/IDiagnosticsManager.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Diagnostics/Instrumentation/DiagnosticInstrumentation.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Diagnostics/Instrumentation/LayoutArrangeInstrumentation.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Diagnostics/Instrumentation/LayoutDiagnosticMetrics.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Diagnostics/Instrumentation/LayoutMeasureInstrumentation.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Graphics/MauiDrawable.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/Button/ButtonHandler.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/Button/ButtonHandler.iOS.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/DatePicker/DatePickerHandler.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/DatePicker/DatePickerHandler.MacCatalyst.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/Editor/EditorHandler.iOS.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/Entry/EntryHandler.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/Entry/EntryHandler.iOS.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/FlyoutView/FlyoutViewHandler.Android.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Standard.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Tizen.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.Windows.cs exists
[2026-06-12 16:52:53] ✓ src/Core/src/Handlers/HybridWebView/HybridWebViewHandler.cs exists
[2026-06-12 16:52:53] ERROR: Fix file not found: src/Core/src/Handlers/HybridWebView/HybridWebViewHelper.cs
UI Tests
Full UI test matrix will run (no specific categories detected from PR changes).
Pre-Flight — Context & Validation
Issue: #34100 - Essentials static Default properties need a public registration mechanism - Every Essentials API (Preferences, FilePicker, SecureStorage, etc.) is inaccessible to custom backends without reflection hacks
PR: #35068 - Bridge DI-registered Essentials implementations to static facades
Platforms Affected: Android, iOS, MacCatalyst, Windows, Tizen, external/custom TFMs; testing platform requested: Android
Files Changed: 1 implementation, 4 test
Key Findings
- The PR registers
EssentialsInitializerfromUseEssentials()so DI-registered Essentials interfaces are copied into existing static facades duringMauiApp.Build()without public API additions. - Linked issue #34100 asks for an official way for custom platform backends to make static Essentials APIs use DI implementations instead of reference-assembly stubs.
- Gate verification was already completed and failed before test execution because the baseline verifier saw unrelated branch-wide changed files and stopped on missing
src/Core/src/Handlers/HybridWebView/HybridWebViewHelper.cs; gate was not re-run. - Independent code review found a correctness risk: the bridge only sets facades when DI has a service, so a later
MauiAppwithout a registration can inherit a previous app's bridged static instance in the same process.
Code Review Summary
Verdict: NEEDS_CHANGES
Confidence: low
Errors: 1 | Warnings: 0 | Suggestions: 0
Key code review findings:
- ❌
src/Core/src/Hosting/EssentialsMauiAppBuilderExtensions.cs:317-319—BridgeIfRegisteredleaves stale static facades across repeatedMauiApp.Build()calls when a prior app registered an Essentials implementation and a later app does not.
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| PR | PR #35068 | Register EssentialsInitializer unconditionally and bridge every DI-registered Essentials interface to its static facade during app initialization. |
❌ FAILED (Gate pre-check did not reach tests) | src/Core/src/Hosting/EssentialsMauiAppBuilderExtensions.cs, unit tests |
Original PR; review found stale static facade risk. |
Code Review — Deep Analysis
Code Review — PR #35068
Independent Assessment
What this changes: Bridges DI-registered Essentials services into existing static facades during MauiApp.Build(), plus serializes affected static-state tests.
Inferred motivation: Let custom Essentials implementations registered in DI back APIs like Preferences.Default without separately calling internal static setters.
Reconciliation with PR Narrative
Author claims: PR addresses DI/static facade bridging for issue #34100, with guards for known platform callback hazards and tests.
Agreement/disagreement: The core bridge matches the stated goal, and prior platform-specific hazards were addressed. One lifecycle/static-state case remains uncovered.
Prior Review Reconciliation
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
| MainThread initializer order | kubaflo | ✅ Fixed | MainThreadBridgeInitializer registered before EssentialsInitializer on external TFMs. |
IAppActions missing platform contract |
github-actions/MauiBot | ✅ Fixed | Guarded by IPlatformAppActions before AppActions.SetCurrent. |
IActivityStateManager late Android bridge |
MauiBot | ✅ Fixed | No longer bridged; explanatory comment present. |
IWebAuthenticator missing platform callback contract |
MauiBot | ✅ Fixed | Guarded by IPlatformWebAuthenticatorCallback on native callback platforms. |
| MainThread dispatch return value | MauiBot | 🔄 Obsolete | Current PR scope defers MainThread behavior to already-merged code; not introduced by current diff. |
Blast Radius Assessment
- Runs for all instances: yes —
EssentialsInitializeris registered unconditionally inUseEssentials. - Startup impact: yes — resolves many optional DI services during
MauiApp.Build(). - Static/shared state: yes — writes process-wide Essentials static backing fields.
CI Status
- Required-check result:
gh pr checks --requiredunavailable due missing auth; public head check-runs showmaui-prfailing. - Classification: undetermined for this environment; confidence capped low.
- Action taken: recorded tool/auth gap; did not post comments.
Findings
❌ Error — DI bridge leaves stale static facades across app builds
src/Core/src/Hosting/EssentialsMauiAppBuilderExtensions.cs:317-319
BridgeIfRegistered only assigns a static facade when the current app has a DI registration. If one MauiApp registers IPreferences, builds, and later another MauiApp in the same process builds without IPreferences, Preferences.Default still points at the first app's DI instance. That can preserve a disposed/stale app-scoped service and contradicts the “If not registered, lazy platform default behavior is preserved” behavior. Existing tests start from reflection-cleared state, so they miss the build-after-bridged case.
Failure-Mode Probing
- Build app A with DI then app B without DI: B inherits A's static Essentials implementation.
- DI service is transient/scoped/disposed: static facade may hold an invalid lifetime beyond its provider.
- Native callback guards:
IAppActions/IWebAuthenticatorare guarded; prior platform crash findings appear fixed.
Verdict: NEEDS_CHANGES
Confidence: low (shared startup/static-state blast radius and CI/gate unavailable in this environment)
Summary: The bridge mostly follows MAUI patterns and addresses prior platform findings, but stale static state across repeated app builds is a concrete correctness issue that should be fixed or explicitly designed/tested before merge.
Fix — Analysis & Comparison
Fix Candidates
| # | Source | Approach | Test Result | Files Changed | Notes |
|---|---|---|---|---|---|
| 1 | try-fix | App-scoped static facade reconciliation lease clears stale Essentials statics when the current app lacks a DI registration. | ✅ PASS for targeted unit tests; |
2 files | Better than PR for stale static-state failure mode; should add ownership-aware cleanup before production use. |
| PR | PR #35068 | Unconditionally register EssentialsInitializer and bridge DI-registered Essentials interfaces to static facades with BridgeIfRegistered. |
❌ FAILED (Gate pre-check did not reach tests) | 5 files | Original PR leaves stale static facade risk across repeated app builds. |
Cross-Pollination
| Model | Round | New Ideas? | Details |
|---|---|---|---|
| maui-expert-reviewer | 1 | Yes | Proposed app-scoped static facade reconciliation lease that clears missing registrations and records cleanup. |
Exhausted: No — stopped because Candidate #1 passed the available regression tests and is demonstrably better than the PR's current fix for the reviewed failure mode. Android-specific validation was attempted but blocked by restore configuration.
Selected Fix: Candidate #1 — It fixes the stale static facade issue found during code review while preserving the PR's DI bridge goal and existing Android callback guards.
Report — Final Recommendation
Comparative Report — PR #35068
Inputs
| Candidate | Source | Result | Notes |
|---|---|---|---|
pr |
Raw PR #35068 | ❌ Failed gate / needs changes | Gate failed before tests, and expert review found a real correctness issue in the bridge. |
pr-plus-reviewer |
Raw PR + expert reviewer feedback applied in sandbox | ✅ Passed targeted regression subset | Applies the STEP 5a reconciliation patch to clear missing DI registrations and adds repeated-build coverage. |
try-fix-1 |
STEP 5a candidate | ✅ Passed targeted regression subset | Same reconciliation approach as the sandboxed reviewer-applied patch; Android-targeted invocation was blocked by restore configuration, not by candidate logic. |
Candidate analysis
pr
The raw PR solves the requested DI-to-static bridge for the happy path and includes useful platform guards. However, it leaves static Essentials facades untouched when a later app does not register a service. Because Essentials facades are process-wide static state, repeated app builds can leak one app's DI backend into another app. The existing PR tests reset static fields between tests and do not cover that lifecycle.
pr-plus-reviewer
This candidate applies the expert reviewer's actionable feedback by reconciling each facade with the current app's DI container: registered services are assigned, and missing services clear the static facade back to normal lazy platform-default behavior. It passed the targeted unit-test subset and covers both SetDefault and SetCurrent repeated-build stale-state cases. The remaining concern is cleanup ownership across overlapping apps, inherited from the STEP 5a patch, but this candidate is still materially safer than the raw PR.
try-fix-1
This is the STEP 5a implementation of the same reconciliation idea. It passed the targeted regression subset and is demonstrably better than the raw PR for the reviewed failure mode. Its Android-targeted test command was blocked by restore configuration (net10.0-android36.0 duplicate framework input), so Android-specific validation remains incomplete. The residual ownership-aware cleanup concern remains.
Ranking
pr-plus-reviewer— best candidate because it preserves the PR's intended DI bridge, incorporates expert feedback, passes the available regression subset, and is the PR-lineage candidate ready for author follow-up.try-fix-1— equivalent technical direction and also passed targeted tests, but it remains an external STEP 5a candidate rather than the PR-plus-reviewer evaluation output.pr— ranked below passing candidates because gate failed and expert review identified an unresolved correctness issue.
Winner
Winner: pr-plus-reviewer
pr-plus-reviewer wins because candidates with passing targeted regression results must rank above the raw PR's failed/needs-changes state, and the reviewer-applied sandbox directly addresses the stale static facade failure while preserving the PR's platform guards. Before merging, the cleanup should be made ownership-aware or simplified so overlapping app disposal cannot clear a newer bridge assignment.
Future Action — review latest findings
No alternative fix was selected for this run. Review the session findings and CI results before merging.
MauiBot
removed
the
s/agent-review-in-progress
|
/review tests |
Tests Failure Analysis
Test Failure Review: Likely unrelated - click to expandOverall verdict: Likely unrelated The
Recommended actionNo action needed from this PR; all failures are pre-existing on the Evidence details
|
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
…5068) <!-- Please let the below note in for people that find this PR --> > [!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](https://github.com/dotnet/maui/wiki/Testing-PR-Builds) from this PR and let us know in a comment if this change resolves your issue. Thank you! ## Description This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface. During `MauiApp` initialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g., `Preferences.Default`, `AppInfo.Current`). This means platforms can simply register their implementations: ```csharp builder.Services.AddSingleton<IPreferences>(new LinuxPreferences()); builder.Services.AddSingleton<IBattery>(new LinuxBattery()); ``` And they'll be available through the standard static APIs (`Preferences.Default`, `Battery.Default`, etc.) without any additional setup. ### How it works - `UseEssentials()` (called automatically for all MAUI apps) now registers `EssentialsInitializer` - During initialization, `BridgeEssentialsFromDI()` resolves each of the 37 Essentials interfaces from DI - If a service is registered, it's bridged to the static facade via the internal `SetDefault`/`SetCurrent` methods - If not registered, the existing lazy platform-default behavior is preserved (no change) ### Alternative approach This is an alternative to #34229 which proposed making `SetDefault()`/`SetCurrent()` public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model. ## Issues Fixed Addresses #34100 ## Tests 10 unit tests covering: - SetDefault pattern types (Preferences, Battery) - SetCurrent pattern types (AppInfo, Connectivity) - Geocoding naming quirk (SetCurrent method + Default property + defaultImplementation field) - No-registration leaves backing fields null (both SetDefault and SetCurrent types) - Multiple simultaneous DI registrations - ConfigureEssentials compatibility - Transient DI lifetime --------- Co-authored-by: Jakub Florkowski <kubaflo123@gmail.com>
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
This PR enables third-party platforms (Linux, macOS non-Catalyst, etc.) to provide custom Essentials implementations via DI without needing new public API surface.
During
MauiAppinitialization, any Essentials interfaces registered in DI are automatically bridged to their corresponding static facades (e.g.,Preferences.Default,AppInfo.Current). This means platforms can simply register their implementations:And they'll be available through the standard static APIs (
Preferences.Default,Battery.Default, etc.) without any additional setup.How it works
UseEssentials()(called automatically for all MAUI apps) now registersEssentialsInitializerBridgeEssentialsFromDI()resolves each of the 37 Essentials interfaces from DISetDefault/SetCurrentmethodsAlternative approach
This is an alternative to #34229 which proposed making
SetDefault()/SetCurrent()public. This DI-first approach avoids new public API surface and aligns with the MAUI hosting model.Issues Fixed
Addresses #34100
Tests
10 unit tests covering: