Add checkpoint/resume system for PR review agent

[kubaflo]

Introduces a checkpoint/resume workflow for the PR review agent to handle environment limitations during testing. Updates the main agent instructions to reference the new checkpoint system and provides detailed guidelines for creating, documenting, and resuming checkpoints in .github/instructions/pr-reviewer-agent/checkpoint-resume.md.

[dotnet-policy-service]

Hey there @@kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[kubaflo]

PR Review: #32081 - [iOS] Added support for large titles in Shell

PR: #32081
Issue: #12156
Reviewer: GitHub Copilot PR Reviewer
Review Date: 2025-11-20

Summary

PR #32081 implements iOS large navigation bar titles in Shell by adding UpdateLargeTitles() to ShellItemRenderer.cs. The implementation correctly maps .NET MAUI's LargeTitleDisplayMode to iOS UINavigationItemLargeTitleDisplayMode.

Status: ✅ Excellent implementation with minor cosmetic cleanup suggested

Core implementation: ✅ Solid - proper null checks, iOS version guard, correct enum mapping, appropriate lifecycle hooks
Test coverage: ✅ Well-structured with screenshot verification, has minor redundancy in preprocessor directive

---

Code Review

✅ Positive Aspects

1. Correct iOS version check

   • Uses OperatingSystem.IsIOSVersionAtLeast(11) (large titles introduced in iOS 11)
   • Prevents crashes on older iOS versions

2. Proper null safety

   • Checks page is null before proceeding
   • Checks navigationController and top before accessing

3. Appropriate lifecycle hooks

   • Called in OnDisplayedPageChanged() when page changes
   • Called in ViewWillLayoutSubviews() to ensure state consistency

4. Correct enum mapping

   LargeTitleDisplayMode.Always => UINavigationItemLargeTitleDisplayMode.Always,
   LargeTitleDisplayMode.Automatic => UINavigationItemLargeTitleDisplayMode.Automatic,
   _ => UINavigationItemLargeTitleDisplayMode.Never

5. Sets both required properties

   • NavigationBar.PrefersLargeTitles (enables feature)
   • NavigationItem.LargeTitleDisplayMode (per-page control)

6. Follows existing patterns

   • Mirrors UpdateTabBarHidden() implementation
   • Consistent with Shell handler architecture

---

🟡 Minor Issues

1. Redundant Conditional Compilation

File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue12156.cs
Line: 1

Issue:

#if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS

TEST_FAILS_ON_ANDROID appears twice - this is redundant but harmless.

How it works: The test project system defines constants for platforms where tests SHOULD NOT run:

• iOS.Tests defines: TEST_FAILS_ON_ANDROID, TEST_FAILS_ON_WINDOWS, TEST_FAILS_ON_CATALYST
• Android.Tests defines: TEST_FAILS_ON_IOS, TEST_FAILS_ON_WINDOWS, TEST_FAILS_ON_CATALYST

So this condition:

• iOS tests: TRUE && TRUE && TRUE = TRUE → test runs ✅
• Android tests: FALSE && FALSE && TRUE = FALSE → test skipped ✅
• Windows tests: FALSE && FALSE && FALSE = FALSE → test skipped ✅

Fix (optional cleanup):

#if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS  // Remove duplicate

Impact: LOW - Code compiles and runs correctly, just has a cosmetic redundancy

---

🟡 Suggestions for Improvement

1. Missing Dynamic Property Change Handling

Current behavior: UpdateLargeTitles() is only called when:

• Displayed page changes (OnDisplayedPageChanged)
• View lays out subviews (ViewWillLayoutSubviews)

Gap: If a user changes Page.LargeTitleDisplay property dynamically at runtime (e.g., in response to user action), the navigation bar won't update.

Comparison: OnDisplayedPagePropertyChanged only listens for Shell.TabBarIsVisibleProperty:

void OnDisplayedPagePropertyChanged(object sender, PropertyChangedEventArgs e)
{
    if (e.PropertyName == Shell.TabBarIsVisibleProperty.PropertyName)
        UpdateTabBarHidden();
    // Missing: Check for LargeTitleDisplay property changes
}

Suggested improvement:

void OnDisplayedPagePropertyChanged(object sender, PropertyChangedEventArgs e)
{
    if (e.PropertyName == Shell.TabBarIsVisibleProperty.PropertyName)
        UpdateTabBarHidden();
    else if (e.PropertyName == PlatformConfiguration.iOSSpecific.Page.LargeTitleDisplayProperty.PropertyName)
        UpdateLargeTitles();
}

Impact: MEDIUM - Users might want to toggle this dynamically (though uncommon)

---

2. Performance Consideration - ViewWillLayoutSubviews

Observation: UpdateLargeTitles() is called in ViewWillLayoutSubviews(), which executes on every layout pass (very frequently).

Analysis:

• Setting navigation bar properties repeatedly could be inefficient
• However, UpdateTabBarHidden() already uses this pattern
• iOS likely optimizes no-op property sets

Potential optimization (optional):

LargeTitleDisplayMode? _lastLargeTitleDisplayMode;

void UpdateLargeTitles()
{
    var page = _displayedPage;
    if (page is null || !OperatingSystem.IsIOSVersionAtLeast(11))
        return;

    var largeTitleDisplayMode = page.OnThisPlatform().LargeTitleDisplay();
    
    // Only update if changed
    if (_lastLargeTitleDisplayMode == largeTitleDisplayMode)
        return;
        
    _lastLargeTitleDisplayMode = largeTitleDisplayMode;
    
    // ... rest of implementation
}

Impact: LOW - Likely not a real performance issue, iOS handles repeated property sets efficiently

---

3. Test Coverage Gaps

Current test:

• Only tests LargeTitleDisplay="Always"
• Uses screenshot verification (good)
• Simple single-page Shell

Missing scenarios:

1. LargeTitleDisplay="Never" - should show regular title
2. LargeTitleDisplay="Automatic" - should follow system behavior
3. Multi-page navigation - title mode changes when navigating
4. Dynamic property changes - changing mode at runtime
5. Shell with multiple tabs - each tab can have different modes

Suggested additional tests (future enhancement):

[Test]
public void LargeTitleDisplayNever()
{
    // Verify standard title bar height
}

[Test]
public void LargeTitleDisplayAutomatic()
{
    // Verify automatic behavior (large on first page, small after scroll/navigation)
}

Impact: MEDIUM - Current test validates basic functionality, but edge cases untested

---

🛑 Testing Status - CHECKPOINT REQUIRED

Environment Limitation

Issue: PR review is being performed in a Linux environment without iOS simulators.
Required: iOS simulator or physical device to validate:

1. Large title actually displays correctly
2. Title size changes between Always/Never/Automatic modes
3. Navigation between pages preserves settings
4. No visual glitches or layout issues

Checkpoint for iOS Testing

Platform: iOS 17+ (iPhone Xs or later recommended)
Why needed: Visual validation of large title behavior requires running on iOS

Testing steps needed:

1. Build Sandbox app with test scenario:

   # Modify src/Controls/samples/Controls.Sample.Sandbox/MainPage.xaml to:
   # - Create a Shell with LargeTitleDisplay="Always"
   # - Add navigation button to test with LargeTitleDisplay="Never"
   # - Add another button for "Automatic" mode
   
   # Build for iOS
   dotnet build src/Controls/samples/Controls.Sample.Sandbox/Maui.Controls.Sample.Sandbox.csproj -f net10.0-ios
   
   # Install to simulator
   xcrun simctl install $UDID artifacts/bin/Maui.Controls.Sample.Sandbox/Debug/net10.0-ios/iossimulator-arm64/Maui.Controls.Sample.Sandbox.app
   
   # Launch and capture screenshots
   xcrun simctl launch --console-pty $UDID com.microsoft.maui.sandbox

2. Visual verification:

   • Screenshot with LargeTitleDisplay="Always" - title should be LARGE
   • Screenshot with LargeTitleDisplay="Never" - title should be regular size
   • Screenshot with LargeTitleDisplay="Automatic" - title should be large initially
   • Navigate to second page - verify title behavior changes as expected

3. Edge case testing:

   • Change property dynamically at runtime (if implemented)
   • Navigate back and forth between pages
   • Switch between Shell tabs (if multi-tab)

Resume instructions: After iOS testing is complete, update this review document with:

• Test results (pass/fail for each scenario)
• Screenshots showing large title vs regular title
• Any visual issues discovered

---

Current Recommendation

✅ Approve with Optional Improvements

Optional Cleanup (Non-Blocking)

1. 🟡 Remove redundant condition in UI test (Line 1 of Issue12156.cs)
   • Current: #if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS
   • Cleaner: #if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS
   • Impact: Cosmetic only - code works correctly either way

Recommended Improvements (Non-Blocking)

1. 🟡 Add property change listener for dynamic LargeTitleDisplay updates (see suggestion above)
2. 🟡 Expand test coverage to include Never/Automatic modes (future enhancement)
3. 🟡 Consider caching last display mode to avoid redundant property sets (optional performance optimization)

---

Post-iOS-Testing Update

iOS testing status: Unable to complete due to Linux environment limitation (no iOS simulators available)

Recommendation without iOS testing: Based on code review alone, the implementation appears correct:

• Proper use of iOS APIs
• Correct lifecycle integration
• Appropriate null safety and version checks
• Follows established patterns in the codebase

Ideal validation (when iOS environment is available):

1. Visual confirmation that large titles display correctly
2. Verification of Always/Never/Automatic modes
3. Test navigation between pages with different settings
4. Confirm no visual glitches or layout issues

Current recommendation: ✅ Approve - Code quality is excellent, iOS testing would provide additional confidence but is not blocking

---

Related Documentation

• iOS Large Title Documentation
• UINavigationBar.prefersLargeTitles
• .NET MAUI Platform Specifics - iOS

---

Review History

• 2025-11-20: Initial code review complete, iOS testing checkpoint created
• Pending: iOS simulator validation

[kubaflo]

PR Review: #32081 - iOS Large Titles in Shell

Summary

PR correctly implements large title support for Shell navigation on iOS by adding an UpdateLargeTitles() method to ShellItemRenderer that mirrors the existing pattern from NavigationRenderer. The implementation properly handles the iOS platform-specific LargeTitleDisplay setting and applies it to the navigation bar.

Recommendation: ⚠️ Request Changes - Test file has critical compilation error that must be fixed

Code Review

Implementation Analysis

What the PR fixes: Issue #12156 where setting ios:Page.LargeTitleDisplay="Always" on Shell pages had no effect. Large titles are an iOS-specific feature introduced in iOS 11 that displays enlarged, bold navigation titles.

Why the fix works:

1. Root Cause: ShellItemRenderer was not reading or applying the LargeTitleDisplay platform-specific property, even though the property existed in the MAUI API. The NavigationPage handler (NavigationRenderer) already had this support, but Shell did not.

2. Solution Approach: Added UpdateLargeTitles() method following the same pattern as NavigationRenderer:

   • Reads the LargeTitleDisplay value from the displayed page using .OnThisPlatform().LargeTitleDisplay()
   • Sets NavigationBar.PrefersLargeTitles (enables/disables large title capability on the nav bar)
   • Sets NavigationItem.LargeTitleDisplayMode on the top view controller (controls per-page large title display)

3. Call Sites: Method is called in two critical locations:

   • OnDisplayedPageChanged(): When Shell navigates to a new page
   • ViewWillLayoutSubviews(): When view hierarchy updates (ensures consistency during layout changes)

Code Quality:

✅ Strengths:

• Follows existing NavigationRenderer pattern exactly (consistency)
• Proper iOS version check (IsIOSVersionAtLeast(11))
• Null-safety checks on page and top view controller
• Uses modern C# switch expression
• Correctly sets both NavigationBar.PrefersLargeTitles AND NavigationItem.LargeTitleDisplayMode (many implementations miss one or the other)

✅ Platform-Specific Code:

• Properly isolated to iOS handler
• Uses correct namespace Microsoft.Maui.Controls.PlatformConfiguration.iOSSpecific
• Follows .NET MAUI platform-specific conventions

Edge Cases Considered:

✅ Handled Correctly:

• Page is null (early return)
• iOS version < 11 (early return, large titles not supported)
• SelectedViewController is not UINavigationController (safely ignored)
• TopViewController is null (null-conditional operator prevents crash)

Potential Issues (Low severity, not blocking):

1. Multiple Updates: ViewWillLayoutSubviews() can be called frequently. Since the method only reads properties and sets UIKit values (idempotent operations), this shouldn't cause performance issues, but it's worth noting.

2. Property Change Handling: Unlike NavigationRenderer, this doesn't listen for LargeTitleDisplayProperty.PropertyChanged. If a page dynamically changes its LargeTitleDisplay value after being displayed, it might not update. However:

   • This matches Shell's general pattern of reading values on navigation
   • Dynamic title display changes are uncommon in real apps
   • Not a regression (feature never worked before)

Test Coverage Review

UI Test Files

HostApp (Issue12156.xaml): ✅ Good

• Properly uses Shell with ios:Page.LargeTitleDisplay="Always"
• Includes AutomationId="Label" for test verification
• Demonstrates the feature correctly

Code-Behind (Issue12156.xaml.cs): ✅ Good

• Proper [Issue] attribute with correct tracker, number, description, and platform
• Minimal, focused implementation

NUnit Test File (Issue12156.cs): 🔴 CRITICAL ISSUE

Line 1 has compilation error:

#if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS

Problem: Duplicate condition TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_ANDROID

Correct Fix (choose ONE of these approaches):

Option 1 - iOS only (matches issue #12156 which is iOS-specific):

#if !ANDROID && !WINDOWS

Option 2 - Explicit iOS only:

#if IOS

Option 3 - If test should actually fail on non-iOS (per naming):

#if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS

Recommendation: Use Option 1 (#if !ANDROID && !WINDOWS) since:

• Large titles are iOS-specific (Android and Windows don't have this feature)
• Matches the PlatformAffected.iOS in the Issue attribute
• Consistent with other iOS-only tests in the repository

Test Quality:

• ✅ Proper inheritance from _IssuesUITest
• ✅ Correct [Category(UITestCategories.TitleView)]
• ✅ Uses VerifyScreenshot() for visual verification (appropriate for title display)
• ⚠️ Missing: Test doesn't verify different LargeTitleDisplayMode values (Always, Never, Automatic). Consider adding test cases for each mode.

Issues Found

🔴 Critical (Must Fix Before Merge)

Issue 1: Compilation Error in Test

• File: src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue12156.cs
• Line: 1
• Problem: Duplicate TEST_FAILS_ON_ANDROID in conditional compilation directive
• Impact: Test file will not compile
• Fix: Change line 1 to #if !ANDROID && !WINDOWS

💡 Suggestions (Optional Improvements)

Suggestion 1: Test Coverage for All Display Modes

Currently, the test only covers LargeTitleDisplay="Always". Consider adding test cases for:

• LargeTitleDisplay="Never" (should show small title)
• LargeTitleDisplay="Automatic" (iOS decides based on context)

Example structure:

[Test]
[Category(UITestCategories.TitleView)]
public void LargeTitleDisplayAlways()
{
    // Navigate to page with Always setting
    App.WaitForElement("AlwaysLabel");
    VerifyScreenshot("LargeTitle_Always");
}

[Test]
[Category(UITestCategories.TitleView)]
public void LargeTitleDisplayNever()
{
    // Navigate to page with Never setting
    App.WaitForElement("NeverLabel");
    VerifyScreenshot("LargeTitle_Never");
}

Suggestion 2: Dynamic Title Changes

Consider testing if dynamically changing LargeTitleDisplay after navigation works. This could be a follow-up enhancement if needed.

Suggestion 3: Shell Navigation Scenarios

Test with multiple Shell pages to ensure large title settings persist correctly during Shell navigation.

Comparison with NavigationRenderer

The implementation correctly mirrors NavigationRenderer.UpdateLargeTitles() with one key difference:

NavigationRenderer:

void UpdateLargeTitles()
{
    var page = Child;
    if (page != null && OperatingSystem.IsIOSVersionAtLeast(11))
    {
        var largeTitleDisplayMode = page.OnThisPlatform().LargeTitleDisplay();
        switch (largeTitleDisplayMode)
        {
            case LargeTitleDisplayMode.Always:
                NavigationItem.LargeTitleDisplayMode = UINavigationItemLargeTitleDisplayMode.Always;
                break;
            // ... more cases
        }
    }
}

ShellItemRenderer (this PR):

void UpdateLargeTitles()
{
    var page = _displayedPage;
    if (page is null || !OperatingSystem.IsIOSVersionAtLeast(11))
        return;

    var largeTitleDisplayMode = page.OnThisPlatform().LargeTitleDisplay();

    if (SelectedViewController is UINavigationController navigationController)
    {
        navigationController.NavigationBar.PrefersLargeTitles = largeTitleDisplayMode != LargeTitleDisplayMode.Never;
        var top = navigationController.TopViewController;
        if (top is not null)
        {
            top.NavigationItem.LargeTitleDisplayMode = largeTitleDisplayMode switch
            {
                LargeTitleDisplayMode.Always => UINavigationItemLargeTitleDisplayMode.Always,
                LargeTitleDisplayMode.Automatic => UINavigationItemLargeTitleDisplayMode.Automatic,
                _ => UINavigationItemLargeTitleDisplayMode.Never
            };
        }
    }
}

Key Differences (all appropriate for Shell context):

1. NavigationBar.PrefersLargeTitles: Shell version sets this, NavigationRenderer doesn't (Shell needs explicit opt-in)
2. Switch vs Pattern: Shell uses modern switch expression, NavigationRenderer uses classic switch (both work fine)
3. Null pattern: Shell uses is null pattern, NavigationRenderer uses != null (stylistic difference)

All differences are appropriate for their respective contexts.

Security Considerations

✅ No security concerns:

• No user input processing
• No file I/O or network operations
• No SQL or external data access
• Only reads MAUI property and sets UIKit navigation bar properties

Breaking Changes

✅ No breaking changes:

• New functionality only, no existing API changes
• Additive change (enables previously non-functional feature)
• No public API modifications

Documentation

⚠️ Missing: XML documentation on UpdateLargeTitles() method.

While not strictly required for private methods, adding a summary would help future maintainers:

/// <summary>
/// Updates the navigation bar's large title display mode based on the currently displayed page's
/// iOS platform-specific LargeTitleDisplay setting. Only affects iOS 11+.
/// </summary>
void UpdateLargeTitles()
{
    // ... implementation
}

Testing Validation

Unable to perform live device testing in this environment. Recommend the following manual validation:

1. Basic Functionality:

   • Run Issue12156 test on iOS simulator (iOS 16+)
   • Verify large title appears in navigation bar
   • Screenshot test should show large, bold title

2. Edge Cases to Test:

   • Navigate between pages with different LargeTitleDisplay values
   • Test on iOS 11, 16, and latest iOS versions
   • Verify behavior when changing Shell's displayed page
   • Test with Shell FlyoutBehavior variations

3. Regression Testing:

   • Verify NavigationPage large titles still work (shouldn't be affected)
   • Check that Shell without LargeTitleDisplay set shows default behavior

Recommendation

⚠️ Request Changes

Required Before Merge:

1. Fix compilation error in Issue12156.cs (line 1 conditional directive)

After fix applied:

• Code implementation is excellent and ready for merge
• Test will be valid once compilation error is fixed
• Consider suggestions for enhanced test coverage in future PRs

Positive Feedback

✅ Excellent work:

• Clean, focused implementation following established patterns
• Proper error handling and edge case coverage
• Good test coverage with visual verification
• Solves a 2-year-old issue (created Dec 2022)

The implementation demonstrates strong understanding of both MAUI and iOS UIKit navigation patterns.

[Copilot]

Pull Request Overview

This PR introduces a checkpoint/resume workflow system for the PR review agent to handle cases where testing cannot be completed due to environment limitations (e.g., missing physical devices, unavailable platforms, specific OS versions). The changes enhance the agent instructions with improved workflow guidance, common mistake documentation, and detailed checkpoint procedures.

Key changes:

• Adds new checkpoint/resume system documentation with examples and best practices
• Updates agent instructions to emphasize reading instructions before creating plans
• Adds comprehensive "common mistakes" guidance based on lessons learned from PR #32479
• Includes detailed documentation of lessons learned and proposed instruction improvements

Reviewed Changes

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

Show a summary per file

File	Description
.github/agents/pr-reviewer.md	Updates workflow instructions to add checkpoint/resume reference and enforces reading instructions first; contains formatting issues (nested list numbering, unclosed code fence)
.github/instructions/pr-reviewer-agent/testing-guidelines.md	Adds prominent section on app selection (Sandbox vs HostApp) with decision trees and cost analysis; has non-standard frontmatter formatting
.github/instructions/pr-reviewer-agent/checkpoint-resume.md	New file introducing complete checkpoint/resume system with format templates, examples, and best practices; contains nested code fence issues in template section
.github/instructions/pr-reviewer-agent/core-guidelines.md	Adds "Critical Success Factors" section with mandatory checkpoints and testing workflow guidance
.github/instructions/pr-reviewer-agent/error-handling.md	Adds comprehensive "Common Mistakes" section with checklists for each review phase
.github/instructions/pr-reviewer-agent/proposed-instruction-improvements.md	Documents proposed improvements based on lessons learned, serving as implementation reference
.github/instructions/pr-reviewer-agent/lessons-learned-pr-32479.md	Comprehensive post-mortem of PR review session documenting what went wrong and right

[Copilot]

There's an unclosed code fence. Line 57 has three backticks (```) which appears to be opening a code block, but there's no corresponding closing fence. This could cause rendering issues in Markdown. Either remove this line if it was added accidentally, or close the code block properly if content is meant to follow.

Suggested change

	**See instruction files above for complete details.**
	```
	**See instruction files above for complete details.**

[Copilot]

[nitpick] The frontmatter-like structure at lines 1-3 is non-standard. YAML frontmatter in Markdown should contain key-value pairs (e.g., title: value), but this has prose content instead. If the intention is to display a critical warning at the top of the rendered document, this should either be:

1. Proper Markdown without the triple-dash delimiters (just start with the warning), OR
2. Valid YAML frontmatter with metadata fields

The current format may not render as expected in all Markdown parsers.

Suggested change

	---
	⚠️ **CRITICAL**: Read this ENTIRE file before creating any plans or taking any actions
	---
	⚠️ **CRITICAL**: Read this ENTIRE file before creating any plans or taking any actions

[Copilot]

The numbered list has a structure issue. Line 17 says "2. Read instruction files IN THIS EXACT ORDER:" but then the numbered items restart at 1. This should either:

1. Continue the numbering from line 16 (so items would be 2.1-2.6), OR
2. Remove the numbered list prefix on line 17 and just say "Read instruction files IN THIS EXACT ORDER:"

The current structure has step "1." on line 16, then step "2." on line 17, but then restarts numbering with another "1." on line 19, followed by step "3." on line 25 which refers back to the outer list.

[PureWeen]

/rebase