From 4c02e0efb91a9e5f942760fef5dd8d7ef4e01d1f Mon Sep 17 00:00:00 2001 From: Jakub Florkowski Date: Wed, 19 Nov 2025 00:04:47 +0100 Subject: [PATCH 1/4] Update AI agent usage instructions in README Clarifies usage by splitting instructions into two options: GitHub Copilot CLI (local) and GitHub Copilot Agents (web). Adds step-by-step guidance for using the web interface. --- .github/README-AI.md | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/README-AI.md b/.github/README-AI.md index 5fd40c573579..55687a0abc58 100644 --- a/.github/README-AI.md +++ b/.github/README-AI.md @@ -8,6 +8,8 @@ The PR reviewer agent conducts thorough, constructive code reviews of .NET MAUI ### How to Use +#### Option 1: GitHub Copilot CLI (Local) + ```bash # Start GitHub Copilot CLI with agent support copilot --allow-all-tools --allow-all-paths @@ -19,7 +21,7 @@ copilot --allow-all-tools --allow-all-paths please review ``` -### Example +#### Example ```bash copilot --allow-all-tools --allow-all-paths @@ -27,6 +29,21 @@ copilot --allow-all-tools --allow-all-paths please review https://github.com/dotnet/maui/pull/32372 ``` +#### Option 2: GitHub Copilot Agents (Web) + +1. **Navigate to the agents tab** at https://github.com/copilot/agents + +2. **Select your repository and branch** using the dropdown menus in the text box + +3. **Choose your agent** from the dropdown (pr-reviewer) + +4. **Enter a task** in the text box: + - For PR reviews: `Please review this PR: https://github.com/dotnet/maui/pull/XXXXX` + +5. **Click Start task** or press Return + +6. **Follow the agent's progress** - The agent task will appear below the text box. Click into it to see live updates. + ## What the Agent Does Every PR review includes: From 2ab45607a8d6b8c1c025feb99bba6b4f1a72b1b3 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 19 Nov 2025 15:35:00 -0600 Subject: [PATCH 2/4] - improve instructions # Conflicts: # .github/instructions/common-testing-patterns.md --- .../appium-control.instructions.md | 65 +++++++++++++- .../instructions/common-testing-patterns.md | 87 +++++++++++++++++++ .../pr-reviewer-agent/core-guidelines.md | 37 ++++++++ .../pr-reviewer-agent/testing-guidelines.md | 9 ++ 4 files changed, 197 insertions(+), 1 deletion(-) diff --git a/.github/instructions/appium-control.instructions.md b/.github/instructions/appium-control.instructions.md index 714706dfc372..4d4e7b969f85 100644 --- a/.github/instructions/appium-control.instructions.md +++ b/.github/instructions/appium-control.instructions.md @@ -6,6 +6,12 @@ description: "Quick reference for creating standalone Appium control scripts for Create standalone C# scripts for manual Appium-based debugging and exploration of .NET MAUI apps. Use these when you need direct control outside of automated tests. +**๐Ÿšจ CRITICAL: Appium is the ONLY way to interact with device UI** + +- โœ… **ALWAYS use Appium** for tapping, swiping, finding elements, rotating device, etc. +- โŒ **NEVER use** `adb shell input tap`, `adb shell input swipe`, or coordinate-based commands +- **Why**: Appium provides reliable element location, proper waits, and cross-device compatibility + **Common Command Patterns**: For UDID extraction, device boot, and build patterns, see [Common Testing Patterns](common-testing-patterns.md). ## When to Use @@ -15,6 +21,7 @@ Create standalone C# scripts for manual Appium-based debugging and exploration o - **Investigation** - Reproduce issues or explore edge cases - **Learning** - Understand how Appium interacts with MAUI apps - **Prototyping** - Test Appium interactions before creating full UI tests +- **PR validation** - Testing PRs that affect UI behavior **Not for automated testing** - For automated UI tests, use the established test infrastructure in `src/Controls/tests/`. @@ -255,6 +262,62 @@ dotnet run yourscript.cs **Complete workflow:** See [Quick Start with Sandbox App](#quick-start-with-sandbox-app) above for full end-to-end example. +## โŒ Wrong vs โœ… Right Approach + +### Opening a Flyout Menu + +**โŒ WRONG - Using ADB commands (brittle, unreliable)**: +```bash +# DON'T DO THIS - coordinates are device-specific and unreliable +adb shell input tap 100 100 # Guess where hamburger menu is +sleep 2 # Hope it opened +``` + +**โœ… RIGHT - Using Appium (reliable, verifiable)**: +```csharp +// Find the hamburger menu by its accessibility properties +var flyoutButton = driver.FindElement( + MobileBy.XPath("//android.widget.ImageButton[@content-desc='Open navigation drawer']") +); +flyoutButton.Click(); + +// Verify it actually opened +var flyoutItems = driver.FindElements(MobileBy.XPath("//android.widget.TextView[contains(@text, 'Item')]")); +Console.WriteLine($"Flyout opened with {flyoutItems.Count} items"); +``` + +### Rotating Device + +**โŒ WRONG - Using simctl/ADB**: +```bash +# DON'T DO THIS - doesn't guarantee app orientation changed +adb shell content insert --uri content://settings/system --bind name:s:user_rotation --bind value:i:1 +``` + +**โœ… RIGHT - Using Appium**: +```csharp +// Appium handles rotation properly and waits for completion +driver.Orientation = ScreenOrientation.Landscape; + +// Verify rotation succeeded +if (driver.Orientation != ScreenOrientation.Landscape) +{ + Console.WriteLine("โŒ Rotation failed!"); +} +``` + +### Taking Screenshots + +**โœ… BOTH are acceptable** (Appium preferred for consistency): +```csharp +// Appium (preferred - works cross-platform) +var screenshot = driver.GetScreenshot(); +screenshot.SaveAsFile("/tmp/screenshot.png"); + +// ADB (acceptable for Android-only scenarios) +// adb exec-out screencap -p > screenshot.png +``` + ## Common Appium Operations ### Finding and Interacting with Elements @@ -349,7 +412,7 @@ For Shell-specific testing patterns (e.g., opening flyouts), see [UI Tests Instr **"Device not found" (Android)** - Verify DEVICE_UDID is set: `echo $DEVICE_UDID` - List devices: `adb devices` -- Start emulator via Android Studio or: `emulator -avd [avd-name]` +- Start emulator: See [Common Testing Patterns: Android Emulator Startup](common-testing-patterns.md#android-emulator-startup-with-error-checking) for the correct background daemon pattern **"Appium server keeps stopping"** - Check if port 4723 is already in use: `lsof -i :4723` diff --git a/.github/instructions/common-testing-patterns.md b/.github/instructions/common-testing-patterns.md index 8c7c7aa0c3ad..36a3b15e6218 100644 --- a/.github/instructions/common-testing-patterns.md +++ b/.github/instructions/common-testing-patterns.md @@ -106,6 +106,93 @@ echo "Simulator is booted and ready" --- +### Android Emulator Startup with Error Checking + +**Used in**: PR reviews, investigation work + +**Pattern**: +```bash +# Clean up any existing emulator processes +pkill -9 qemu-system-x86_64 2>/dev/null || true +pkill -9 emulator 2>/dev/null || true +sleep 2 + +# CRITICAL: Start emulator as background daemon that survives session end +# Use subshell with & to fully detach from current session +cd $ANDROID_HOME/emulator && (./emulator -avd Pixel_9 -no-snapshot-load -no-audio -no-boot-anim > /tmp/emulator.log 2>&1 &) + +# Wait a moment for process to start +sleep 3 + +# Verify emulator process started +EMULATOR_PID=$(ps aux | grep "qemu.*Pixel_9" | grep -v grep | awk '{print $2}') +if [ -z "$EMULATOR_PID" ]; then + echo "โŒ ERROR: Emulator failed to start" + exit 1 +fi +echo "โœ… Emulator started as background daemon (PID: $EMULATOR_PID)" + +# Wait for device to appear +echo "Waiting for device to appear..." +adb wait-for-device + +# Wait for boot to complete +echo "Waiting for boot to complete..." +until [ "$(adb shell getprop sys.boot_completed 2>/dev/null)" = "1" ]; do + sleep 2 + echo -n "." +done +echo "" + +# Get device UDID +export DEVICE_UDID=$(adb devices | grep -v "List" | grep "device" | awk '{print $1}' | head -1) + +# Verify device is ready +if [ -z "$DEVICE_UDID" ]; then + echo "โŒ ERROR: Emulator started but device not found" + exit 1 +fi + +# Check API level +API_LEVEL=$(adb shell getprop ro.build.version.sdk) +echo "โœ… Emulator ready: $DEVICE_UDID (API $API_LEVEL)" +``` + +**When to use**: Starting Android emulator for testing + +**Why this pattern is critical**: + +The subshell `()` with `&` pattern is essential for emulator persistence: +- **Problem**: Using `mode="async"` attaches the emulator to the bash session, causing it to be killed when the session ends +- **Root cause**: Background processes attached to sessions are terminated during cleanup, even with `nohup` +- **Solution**: Subshell `()` creates a new process group that's detached from the current session +- **Result**: Emulator persists even when AI agent finishes responding or sessions end + +**Wrong approach (emulator dies)**: +```bash +# DON'T DO THIS - emulator will be killed when session ends +bash --mode=async ./emulator -avd Pixel_9 & +``` + +**Correct approach (emulator persists)**: +```bash +# Subshell with & fully detaches the process +cd $ANDROID_HOME/emulator && (./emulator -avd Pixel_9 ... &) +``` + +**Critical details**: +- The emulator command must be run from `$ANDROID_HOME/emulator` directory. Running from other directories causes "Qt library not found" and "qemu-system not found" errors +- **Use subshell `()` with `&`** to start emulator as true background daemon that persists after bash session ends +- **NEVER use `adb kill-server`** - This disconnects ALL active ADB connections and causes emulators to lose connection. Almost never necessary +- **NEVER use `mode="async"` for emulators** - Processes will be terminated when the session ends +- **Check first**: Run `adb devices` before starting - if device is already visible, no action needed + +**Available emulators**: List with `emulator -list-avds` + +**To verify persistence**: After starting emulator, note the PID, finish the current task, then check if the process still exists with `ps aux | grep ` + +--- + ## 3. Build Patterns ### Sandbox App Build (iOS) diff --git a/.github/instructions/pr-reviewer-agent/core-guidelines.md b/.github/instructions/pr-reviewer-agent/core-guidelines.md index 6ab64dedf0f5..5edf9adcb103 100644 --- a/.github/instructions/pr-reviewer-agent/core-guidelines.md +++ b/.github/instructions/pr-reviewer-agent/core-guidelines.md @@ -62,6 +62,43 @@ When multiple instruction files exist, follow this priority order: - This clearly identifies agent-generated PRs containing review feedback and suggested improvements - Example: `[PR-Reviewer] Fix RTL padding for CollectionView on iOS` +## ๐Ÿค– UI Automation: ALWAYS Use Appium + +**CRITICAL RULE: For ANY device UI interaction, use Appium - NEVER use direct ADB/simctl commands** + +**โœ… CORRECT - Use Appium for**: +- Tapping buttons, controls, menu items +- Opening menus, drawers, flyouts +- Scrolling, swiping, gestures +- Entering text +- Rotating device orientation +- Taking screenshots +- Finding and verifying UI elements +- Any interaction a user would perform + +**โŒ WRONG - Never use these for UI interaction**: +- `adb shell input tap` - Use Appium element tap instead +- `adb shell input swipe` - Use Appium gestures instead +- `adb shell input text` - Use Appium SendKeys instead +- `xcrun simctl ui` - Use Appium iOS interactions instead + +**When ADB/simctl ARE acceptable**: +- โœ… `adb devices` - Check device connection +- โœ… `adb logcat` - Monitor logs (read-only) +- โœ… `adb shell getprop` - Read device properties (read-only) +- โœ… `xcrun simctl list` - List simulators +- โœ… `xcrun simctl boot` - Boot simulator +- โœ… Device setup/configuration (not UI interaction) + +**Why this matters**: +- Appium provides reliable element location (no guessing coordinates) +- Appium waits for elements to be ready +- Appium verifies actions succeeded +- Appium works across different screen sizes/orientations +- Coordinate-based taps are brittle and unreliable + +**Reference**: See [Appium Control Scripts Instructions](../appium-control.instructions.md) for creating Appium test scripts + **Why this matters**: Code review alone is insufficient. Many issues only surface when running actual code on real platforms with real scenarios. Your testing often reveals edge cases and issues the PR author didn't consider. **NEVER GIVE UP Principle**: diff --git a/.github/instructions/pr-reviewer-agent/testing-guidelines.md b/.github/instructions/pr-reviewer-agent/testing-guidelines.md index c94c4a3f1139..1869e2ef310d 100644 --- a/.github/instructions/pr-reviewer-agent/testing-guidelines.md +++ b/.github/instructions/pr-reviewer-agent/testing-guidelines.md @@ -158,6 +158,9 @@ fi ``` **Android Testing**: + +**CRITICAL**: If starting an emulator, use the background daemon pattern from [Common Testing Patterns: Android Emulator Startup](../../instructions/common-testing-patterns.md#android-emulator-startup-with-error-checking) to ensure it persists across sessions. + ```bash # Get connected device/emulator export DEVICE_UDID=$(adb devices | grep -v "List" | grep "device" | awk '{print $1}' | head -1) @@ -169,6 +172,12 @@ if [ -z "$DEVICE_UDID" ]; then fi ``` +**Important Android Rules**: +- โœ… **Start emulators with subshell + background**: `cd $ANDROID_HOME/emulator && (./emulator -avd Name ... &)` +- โŒ **NEVER use `adb kill-server`** - This disconnects active emulators and is almost never needed +- โŒ **NEVER use `mode="async"` for emulators** - They will be killed when the session ends +- โœ… **Check `adb devices` first** - If device is visible, no action needed + ## Build and Deploy **iOS**: From c25d876f5856122c21ff52e70b38f043aca77a74 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Wed, 19 Nov 2025 15:40:19 -0600 Subject: [PATCH 3/4] - update prompt file --- .github/prompts/pr-reviewer.prompt.md | 159 ++++++++++++++++++++------ 1 file changed, 123 insertions(+), 36 deletions(-) diff --git a/.github/prompts/pr-reviewer.prompt.md b/.github/prompts/pr-reviewer.prompt.md index a05669447b72..858f0aa4f5d6 100644 --- a/.github/prompts/pr-reviewer.prompt.md +++ b/.github/prompts/pr-reviewer.prompt.md @@ -1,62 +1,149 @@ -# PR Reviewer - Quick Reference - -The `pr-reviewer` agent conducts thorough code reviews with hands-on device/simulator testing and validation. - -## How It Works - -The agent always performs **thorough reviews** which include: - -1. **Code Analysis** - Review code for correctness, style, and best practices -2. **Build & Deploy** - Build the Sandbox app and deploy to simulator/emulator -3. **Real Testing** - Test the PR changes on actual devices with measurements -4. **Before/After Comparison** - Compare behavior with and without PR changes -5. **Edge Case Testing** - Test scenarios not mentioned by the PR author -6. **Documented Results** - Provide review with actual test data and evidence +--- +description: "Conduct thorough PR reviews with hands-on testing on real devices/simulators" +name: pr-reviewer +agent: pr-reviewer +--- + +# PR Reviewer + +Reviews .NET MAUI pull requests through code analysis AND hands-on device testing with instrumentation. + +## What It Does + +**Thorough Testing-Based Reviews:** +1. ๐Ÿ“– Fetches and analyzes PR changes +2. ๐Ÿ—๏ธ Builds Sandbox app with PR modifications +3. ๐Ÿ“ฑ Deploys to iOS/Android simulators +4. ๐Ÿ”ฌ Uses **Appium** for reliable UI interaction (never manual ADB commands) +5. ๐Ÿ“Š Instruments code to capture actual measurements +6. โš–๏ธ Compares WITH/WITHOUT PR changes +7. ๐Ÿงช Tests edge cases PR author may have missed +8. ๐Ÿ“ Documents findings with real data + +**Key Principles:** +- โœ… Uses **Sandbox app** for validation (not TestCases.HostApp) +- โœ… Uses **Appium** for ALL UI interactions (taps, swipes, rotations) +- โœ… Uses **background daemon pattern** for Android emulators (survives sessions) +- โœ… Never kills ADB server unnecessarily +- โœ… Pauses and asks for help if stuck (never gives up and switches to code-only review) ## Usage Examples **Basic review:** ``` -Please review PR #32372 +/pr-reviewer Please review PR #32372 ``` -**Platform-specific testing:** +**Platform-specific:** ``` -Test PR #32205 on iOS 26.0 specifically +/pr-reviewer Test PR #32205 on iOS 26.0 ``` **Cross-platform validation:** ``` -Test PR #12345 on both iOS and Android to verify cross-platform behavior +/pr-reviewer Validate PR #12345 on both iOS and Android ``` -**Margin/Layout Issues:** +**Layout/SafeArea issues:** ``` -Review PR #32205 and instrument the Sandbox app to capture actual frame positions +/pr-reviewer Review PR #32275 and test Shell flyout in landscape with display notch ``` -**Before/After Comparison:** +**Specific scenario:** ``` -Test PR #32205 comparing behavior WITH and WITHOUT the changes +/pr-reviewer Test PR #32205 with RTL layout and capture frame measurements ``` -## What Happens If Build Fails? +**Before/after comparison:** +``` +/pr-reviewer Compare behavior WITH and WITHOUT PR #12345 changes +``` -If the agent encounters build errors during testing: +## What You'll Get + +**Comprehensive Review Document** (`Review_Feedback_Issue_XXXXX.md`) including: + +- โœ… **Code Analysis** - Style, correctness, best practices review +- โœ… **Test Environment** - Platform/version/device used +- โœ… **Actual Measurements** - Console output, frame positions, inset values +- โœ… **Before/After Comparison** - Behavior with and without PR +- โœ… **Edge Cases** - Additional scenarios tested beyond PR description +- โœ… **Screenshots** - Visual evidence when relevant +- โœ… **Issues Found** - Problems discovered through testing +- โœ… **Recommendations** - Accept/reject/modify based on evidence + +## Expectations + +**Timeline:** +- โฑ๏ธ Reviews take 15-45 minutes (building, deploying, testing multiple scenarios) +- โœ… Agent has **unlimited time** - will not rush or skip testing +- โœ… You'll see progress updates as testing proceeds + +**Testing Approach:** +- ๐Ÿค– **Appium-based** - All UI automation through Appium (reliable, verifiable) +- ๐Ÿ“ฑ **Real devices** - Tests on actual iOS simulators or Android emulators +- ๐Ÿ“ **Instrumented** - Adds logging to capture measurements +- ๐Ÿ”„ **Comparative** - Tests both with and without PR changes + +**If Issues Occur:** +- Build fails โ†’ Agent attempts 1-2 fixes, then **pauses** for your input +- Emulator crashes โ†’ Agent reports issue and asks for guidance +- Unexpected behavior โ†’ Agent documents and asks for validation +- **Never silently falls back to code-only review** + +## Common Scenarios + +**SafeArea/Layout PRs:** +- Agent instruments views to capture padding, margins, frame positions +- Tests in portrait and landscape orientations +- Verifies safe area insets are applied correctly +- Measures child vs parent view positions + +**UI Component PRs:** +- Opens Sandbox app with test scenario +- Uses Appium to interact with controls (taps, swipes, text entry) +- Captures screenshots and measurements +- Tests on target platform (iOS, Android, or both) + +**Shell/Navigation PRs:** +- Sets up Shell in Sandbox with flyout items +- Uses Appium to open menus, navigate, test gestures +- Verifies behavior in different orientations +- Tests with and without headers/footers + +**Cross-Platform PRs:** +- Tests same scenario on iOS AND Android +- Compares platform-specific behavior +- Verifies consistent user experience +- Notes any platform differences + +## Tips for Best Results + +โœ… **Be specific** about what to test: +``` +/pr-reviewer Test PR #12345 focusing on RTL layout behavior +``` -1. It will try to fix obvious issues (1-2 attempts) -2. If errors persist, it will **STOP** and ask for your help -3. It will **NOT** silently switch to code-only review -4. You'll get a clear error report with options to proceed +โœ… **Mention platform** if it matters: +``` +/pr-reviewer Test PR #32275 on Android with display notch +``` -This ensures you always know when testing couldn't be completed. +โœ… **Request measurements** for layout issues: +``` +/pr-reviewer Capture actual frame positions and padding values for PR #32205 +``` + +โœ… **Ask for edge cases**: +``` +/pr-reviewer Test PR #12345 including rotation, RTL, and empty state scenarios +``` -## Expected Output +โŒ **Don't rush the agent** - Testing takes time, and thoroughness prevents bugs -Every review includes: +## Related Resources -- **Test Results** - Actual console output and measurements from simulator/device -- **Environment Details** - Which platform/version was tested -- **Comparison** - Behavior with and without PR changes -- **Issues & Suggestions** - Validated through real testing -- **Recommendation** - Based on both code review and hands-on validation +- [Core Guidelines](.github/instructions/pr-reviewer-agent/core-guidelines.md) - Full workflow details +- [Testing Guidelines](.github/instructions/pr-reviewer-agent/testing-guidelines.md) - Platform setup, build patterns +- [Appium Instructions](.github/instructions/appium-control.instructions.md) - UI automation patterns +- [Common Testing Patterns](.github/instructions/common-testing-patterns.md) - Device setup, error checking From 2c8f57929c20105256cc115d8370a918496671a6 Mon Sep 17 00:00:00 2001 From: Shane Neuville Date: Thu, 20 Nov 2025 03:27:38 -0600 Subject: [PATCH 4/4] - emphasize using dotnet 10 and not dotnet-script --- .../appium-control.instructions.md | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/.github/instructions/appium-control.instructions.md b/.github/instructions/appium-control.instructions.md index 4d4e7b969f85..4b815f48e7ab 100644 --- a/.github/instructions/appium-control.instructions.md +++ b/.github/instructions/appium-control.instructions.md @@ -6,6 +6,10 @@ description: "Quick reference for creating standalone Appium control scripts for Create standalone C# scripts for manual Appium-based debugging and exploration of .NET MAUI apps. Use these when you need direct control outside of automated tests. +**๐Ÿšจ CRITICAL: Use .NET 10 Native Scripting (NOT dotnet-script)** + +This repository uses **.NET 10's built-in scripting features** with the `#:package` directive. Do NOT use `dotnet-script` or the old `#r` directive syntax. + **๐Ÿšจ CRITICAL: Appium is the ONLY way to interact with device UI** - โœ… **ALWAYS use Appium** for tapping, swiping, finding elements, rotating device, etc. @@ -97,6 +101,8 @@ The fastest way to experiment with Appium is using the Sandbox app (`src/Control # Run your script (from SandboxAppium/ directory) cd SandboxAppium + + # Run with .NET 10's native scripting (NOT dotnet-script) dotnet run yourscript.cs ``` @@ -123,7 +129,12 @@ Ensure Appium server is running on `http://localhost:4723` before running your s ## Basic Template -**IMPORTANT: Create script files inside project directory (e.g., `SandboxAppium/`), not in repository root or `/tmp`.** +**๐Ÿšจ CRITICAL: .NET 10 Scripting Requirements** + +1. **Use `#:package` directive** (NOT `#r` or dotnet-script syntax) +2. **Create scripts inside project directory** (e.g., `SandboxAppium/`), not in `/tmp` or repository root +3. **Run with `dotnet run`** (NOT `dotnet script` or `dotnet-script`) +4. **Requires .NET 10 SDK** - These scripts use .NET 10's native scripting features ```csharp #:package Appium.WebDriver@8.0.1 @@ -237,6 +248,14 @@ catch (Exception ex) ## Running the Script +**๐Ÿšจ CRITICAL: .NET 10 Native Scripting (NOT dotnet-script)** + +- โœ… **DO**: Use `dotnet run yourscript.cs` (.NET 10 native scripting) +- โœ… **DO**: Use `#:package Appium.WebDriver@8.0.1` directive +- โŒ **DON'T**: Use `dotnet script` or `dotnet-script` commands +- โŒ **DON'T**: Use `#r` directive syntax (that's for dotnet-script, not .NET 10) +- โŒ **DON'T**: Create scripts in `/tmp` or repository root + **โš ๏ธ Important: Create scripts inside project directory (e.g., `SandboxAppium/`), not in `/tmp` or repository root.** ```bash