diff --git a/.config/dotnet-tools.json b/.config/dotnet-tools.json index 10311f4926d5..a06771ae3239 100644 --- a/.config/dotnet-tools.json +++ b/.config/dotnet-tools.json @@ -24,7 +24,7 @@ "rollForward": false }, "microsoft.dotnet.xharness.cli": { - "version": "11.0.0-prerelease.26229.1", + "version": "11.0.0-prerelease.26230.4", "commands": [ "xharness" ], diff --git a/.github/agents/maui-expert-reviewer.md b/.github/agents/maui-expert-reviewer.md new file mode 100644 index 000000000000..56efe5638df5 --- /dev/null +++ b/.github/agents/maui-expert-reviewer.md @@ -0,0 +1,606 @@ +--- +name: maui-expert-reviewer +description: "Reviews .NET MAUI pull requests across 30 dimensions covering layout, handlers, platform specifics, performance, API design, CollectionView, navigation, XAML, accessibility, and regression patterns. Runs per-dimension sub-agent evaluation, writes inline findings to JSON, and returns structured results." +--- + +# MAUI Expert Reviewer + +You review .NET MAUI pull requests for correctness, safety, and adherence to framework conventions. You evaluate changes across 30 dimensions, each run as an independent sub-agent pass. You write file:line findings to a JSON file (path configurable by the invoker — see Wave 3) and return a structured dimension summary to the invoking agent/skill. + +**Scope**: Code review only. Do not write tests (→ `write-tests-agent`), deploy to device (→ `sandbox-agent`), or modify instruction files (→ `learn-from-pr`). + +--- + +## Overarching Principles + +1. **Every bug fix needs a regression test** that reproduces the original issue scenario, not just a generic unit test. +2. **Verify logic against the original reproduction** before and after the fix — a fix that passes new tests but fails the original repro is wrong. +3. **Code must live at the correct abstraction layer** — Core vs Controls, handler vs extension method, shared vs platform-specific. +4. **Hot paths must avoid allocations** — no LINQ, closures, or temporary collections in measure/arrange, scroll, or binding propagation. +5. **Property updates go through the mapper system** (`UpdateValue`) — direct calls bypass `AppendToMapping`/`PrependToMapping` customizations. +6. **Null-check VirtualView and MauiContext** before use in every handler callback — both can be null during lifecycle transitions. +7. **Platform-specific changes must not break other platforms** — scope changes or add cross-platform regression tests. +8. **Check git history before modifying existing code** — understand WHY it was added to avoid re-introducing fixed bugs. + +--- + +## Review Dimensions + +### 1. Layout Measure-Arrange Correctness `[critical]` + +Constraint propagation through parent-child hierarchy, no infinite loops, content size tracking consistency. + +- CHECK: Measure and arrange passes use consistent constraints — a child measured at width=200 must not be arranged at width=300 +- CHECK: Layout invalidation triggers re-measure when parent container size changes +- CHECK: ScrollView content is always re-measured on layout trigger (no aggressive caching) +- CHECK: Padding, margin, and border thickness are subtracted before passing constraints to children +- CHECK: No infinite measure/arrange oscillation from circular size dependencies +- CHECK: ArrangeOverride respects the size returned from MeasureOverride +- CHECK: Collection changes propagate via `Handler.UpdateValue(PropertyName)` at the Controls level, not via `INotifyCollectionChanged` from the platform view — INCC creates tight coupling +- CHECK: Guard against infinite `ContentSize` oscillation on iOS — `MauiScrollView` can loop when content triggers layout→resize→layout cycles; use pixel-level comparison (e.g., `EqualsAtPixelLevel` threshold ~0.0000001pt) to break sub-pixel noise from animations +- CHECK: Don't apply padding in aspect ratio calculations — compute ratio first, then add padding +- CHECK: Test visibility propagation through nested containers with full matrix: 2-level and 3-level nesting, set/unset sequences + +#### Platform notes +- **iOS**: Measure and layout passes must stay in sync; out-of-sync causes redraw failures. Compare sizes at pixel resolution to absorb device-pixel rounding. +- **Android**: RecyclerView item measurement must account for pixel rounding +- **Windows**: WinUI uses NaN conventions for unconstrained dimensions + +### 2. Performance-Critical Path Optimization `[major]` + +Avoiding expensive operations on hot paths: measure/arrange, scrolling, binding propagation, property change notifications. + +- CHECK: No LINQ (`.Where`, `.Select`, `.FirstOrDefault`) on measure/arrange/scroll paths — use indexed `for` loops +- CHECK: No unnecessary allocations (closures, temporary collections, string concatenation) in frequently-called methods +- CHECK: Bindable property change handlers skip layout invalidation when value has not actually changed +- CHECK: Collection iteration uses `Count`/indexer rather than `IEnumerable` allocation when source supports it +- CHECK: Expensive computations called multiple times per layout pass are cached with proper invalidation +- CHECK: Cache JNI property access in locals — Android properties like `Context`, `Resources`, `ContentDescription` cross the Java/C# bridge on every access +- CHECK: Avoid closures that capture UIKit objects — they create GC-tracked references that increase memory pressure; prefer explicit parameter passing or static methods +- CHECK: `ValueTuple.GetHashCode()` may allocate for large tuples — implement `GetHashCode()` explicitly for hash-critical types like `SetterSpecificity` +- CHECK: Public APIs return `IReadOnlyList` or `IReadOnlyCollection` instead of mutable `List` +- CHECK: Use `StringBuilder` or `List` for source generator string building — repeated `+=` is O(n²) +- CHECK: Allocate early, check cheap conditions first — test boolean flags and null checks before allocating strings or doing I/O in mapper callbacks +- CHECK: Don't remove caches without benchmarks — replacements must prove equivalent or better performance via `dotnet-trace`/`speedscope.app` +- CHECK: Flatten nested LINQ into single-level iteration when possible, but benchmark to verify improvement + +### 3. Handler Mapper and Property Patterns `[major]` + +Correct usage of mapper/command-mapper, lifecycle symmetry, and chained mapper handling. + +- CHECK: Property updates go through `Handler.UpdateValue(nameof(Property))`, not direct mapper calls +- CHECK: Mapper registrations use `AppendToMapping`/`PrependToMapping` for extensibility +- CHECK: Handler properties are initialized in correct dependency order +- CHECK: CommandMapper entries return void and use `(handler, view, args)` signature — Commands are for requests (`ScrollTo`, `Remove`), Mapper properties associate with `BindableProperty` values +- CHECK: Platform-specific mapper overrides call base mapper when extending +- CHECK: **ConnectHandler/DisconnectHandler symmetry** — every listener, event handler, or callback registered in `ConnectHandler` must be unregistered in `DisconnectHandler` +- CHECK: Don't null handler references eagerly in `DisconnectHandler` — the view might be removed and re-added (e.g., Shell tab switching); clear subscriptions while keeping weak references alive +- CHECK: On Android, assign per-view state in `AttachedToWindow`/`DetachedFromWindow` rather than constructor to avoid leaks when views are recycled +- CHECK: Use `ModifyMapping` for Controls-layer overrides that override Core behavior, so user-registered mappings aren't silently replaced +- CHECK: Mapper methods must be idempotent — they can be called at any time, not just initial setup; must fully initialize state from scratch +- CHECK: `ConnectHandler` calls `base.ConnectHandler()` before custom setup; `DisconnectHandler` cleans up before calling `base.DisconnectHandler()` — reversed ordering can access disposed resources +- CHECK: Centralize listener instances via static registry for per-CoordinatorLayout or per-DrawerLayout listeners to reduce allocations + +### 4. Architectural Layer Placement `[moderate]` + +Code lives at the correct abstraction: Core vs Controls, handler vs extension method, shared vs platform-specific. + +- CHECK: Platform-agnostic logic lives in shared code, not in platform handler implementations +- CHECK: Extension methods on platform types do not contain business logic belonging in the handler +- CHECK: Cross-cutting behavior uses IView/IElement interfaces, not per-control duplication +- CHECK: Compatibility shim logic stays in the Compatibility layer, not leaked into Core handlers +- CHECK: Navigation logic belongs in Shell/NavigationPage handlers, not in individual page handlers + +### 5. Logic and Correctness Verification `[critical]` + +Catching inverted conditions, off-by-one errors, wrong property usage, or semantic errors. + +- CHECK: Boolean conditions are not inverted (checking `IsVisible` when meaning `!IsVisible`) +- CHECK: Correct property is used when similar ones exist (`RawX` vs `X`, `Bounds` vs `Frame`) +- CHECK: Edge cases handled: empty collections, zero dimensions, null parents, single-item scenarios +- CHECK: Fix is verified against the original issue reproduction, not just a new unit test +- CHECK: Arithmetic handles overflow, division by zero, and negative values +- CHECK: Explicit parentheses in index/position/offset calculations — silent operator-precedence bugs in scroll offset, spacing, or size math are hard to spot + +### 6. Regression Prevention and Test Coverage `[critical]` + +Every bug fix needs a regression test. Modified code must be checked against git history. + +- CHECK: Bug fix includes a regression test reproducing the original issue +- CHECK: Reverted or modified code is checked via git blame for why it was added +- CHECK: Test covers the specific scenario from the issue report, not a generic case +- CHECK: Shared code changes are tested on all affected platforms +- CHECK: Previously-fixed issue numbers are cross-referenced when modifying the same code area +- CHECK: UI tests run on all applicable platforms unless there is a specific technical limitation +- CHECK: Snapshot baselines updated across all platforms when changing background color, font, or layout +- CHECK: Screenshot size matches capture method — a size mismatch means the capture changed, not the rendering +- CHECK: Use `VerifyScreenshot(retryTimeout:)` instead of `Task.Delay` — built-in retry handles animations +- CHECK: Test labels are visible even when content is clipped — position a sentinel element inside the clip boundary to prove content was drawn +- CHECK: Android memory tests use `GetMemoryInfo()` with threshold assertions +- CHECK: Test types match project infrastructure — source-gen tests in `SourceGen.UnitTests.csproj`, not `Xaml.UnitTests.csproj`; tests that don't need `[Values] XamlInflator` shouldn't use it + +#### Frequently Regressed Components + +Use this as a triage guide — PRs touching these warrant extra scrutiny on adjacent scenarios: + +| Component | Key Risk Areas | +|-----------|----------------| +| CollectionView | Layout, scroll position, spacing, cell alignment, Header/Footer | +| Image/Graphics | Aspect ratio, CornerRadius, Background, DrawString | +| Theme/Style | AppThemeBinding, implicit styles, ApplyToDerivedTypes | +| CarouselView | ScrollTo, CurrentItem, ItemSpacing, loop mode | +| Gesture/Tap | TapGestureRecognizer, SwipeView, outside-tap dismiss | +| Button/Entry | Dynamic resize, focus/selection, AppThemeBinding colors | +| Toolbar | Icon color, back button, BarTextColor across modes | +| Shell/TabBar | TabBarIsVisible, Shell crashes, section rendering | + +#### Regression Escalation Patterns + +Lessons from reverted PRs and candidate-branch failures. When a PR touches these areas, apply extra scrutiny. + +- CHECK: **Test the fix scenario AND adjacent scenarios** — most reverts happen because the fix works for the reported issue but breaks a neighboring case; require authors to enumerate adjacent behaviors checked +- CHECK: **Never remove `InternalsVisibleTo` without auditing NuGet consumers** — IVT removal silently breaks community packages depending on internal APIs +- CHECK: **Entry/Editor focus and selection state is fragile** — `CursorPosition`, `SelectionLength`, keyboard show/hide, and focus order interact tightly; verify focus behavior especially when keyboard is dismissed and re-shown +- CHECK: **iOS measurement timing lags behind property changes** — `UIButton.TitleLabel.Bounds` and `UIView` frame values are not updated synchronously; measure the title manually instead of reading `.Bounds` immediately after setting a property +- CHECK: **Template changes need all-template validation** — a fix for `maui` can break `maui-blazor` or `maui-multiproject`; validate against all template IDs +- CHECK: **Candidate-branch PRs must not mix concerns** — don't bundle unrelated flakiness fixes with regression fixes; mixed PRs make bisection impossible +- CHECK: **Major dependency upgrades need broad platform validation** — WindowsAppSDK, platform SDK bumps, etc. must be green on all platforms before merge +- CHECK: **ContentPresenter BindingContext propagation breaks explicit TemplateBindings** — propagating `BindingContext` through `ContentPresenter` overwrites `{TemplateBinding}` values; verify TemplateBinding expressions still resolve after the change + +### 7. Public API Surface Design `[major]` + +Additions, removals, or changes to public APIs for intentionality and forward-compatibility. + +- CHECK: New public APIs have clear use cases — no speculative additions +- CHECK: Deprecated APIs are marked `[Obsolete]` with migration guidance before removal; message ends with a period (iOS Cecil tests enforce this) +- CHECK: API naming follows .NET design guidelines and existing MAUI patterns +- CHECK: Breaking changes are documented with explicit design justification +- CHECK: `PublicAPI.Unshipped.txt` entries match the actual API shape +- CHECK: Adding members to a public interface is a breaking change — use default interface methods, create a versioned interface (`IFoo2`), or add an extension method +- CHECK: Never modify `PublicAPI.Shipped.txt` — to remove a shipped API, copy the line to `Unshipped.txt` prefixed with `*REMOVED*` +- CHECK: Don't expose setters that do nothing — dead setters accumulate API surface that can't be removed later +- CHECK: Public `MauiAppBuilder` extension methods cannot be removed once added — evaluate carefully what belongs on the builder +- CHECK: When replacing types, consider keeping a compatibility class that inherits from the new type to ease Xamarin.Forms migration + +### 8. Async and Threading Safety `[major]` + +Correct async/await, UI thread dispatching, cancellation tokens, and race condition prevention. + +- CHECK: Fire-and-forget async uses `.FireAndForget()` with exception handling, not bare `async void` +- CHECK: Platform view modifications are dispatched to the UI thread from background contexts +- CHECK: CancellationToken is threaded through long-running operations +- CHECK: Async handler operations verify the handler is still connected before applying results +- CHECK: Concurrent access to shared state is protected (lock, Interlocked, or immutable patterns) +- CHECK: Use `Interlocked.Increment` for counters accessed from the UI thread — even "most likely single-threaded" counters need safety for debounce correctness +- CHECK: Don't reset debounce counters to zero — roll over at a high threshold (e.g., 100) to prevent missed update requests from race conditions +- CHECK: Check if already on the main thread before dispatching — unnecessary dispatch adds latency and can reorder operations +- CHECK: `Task.Delay` in tests needs justification — prefer deterministic helpers like `WaitForMainThread()` over arbitrary millisecond waits +- CHECK: Guard `DispatcherQueue` for null on Windows before posting — it can be null if the Window is disposed + +#### Platform dispatch +- **Android**: `platformView.Post()` for UI thread; `Looper.MainLooper` for thread check +- **iOS**: `MainThread.BeginInvokeOnMainThread` or `DispatchQueue.MainQueue` +- **Windows**: `DispatcherQueue.TryEnqueue`; be aware of COM apartment model + +### 9. Null Safety and Defensive Coding `[moderate]` + +Null checks, early returns, and nullable annotations to prevent NREs where object availability timing varies by platform. + +- CHECK: VirtualView is null-checked in handler callbacks — it can be null during disconnect +- CHECK: MauiContext is validated; throw `InvalidOperationException` with descriptive message if null +- CHECK: Platform callbacks guard against null native views (may be collected or disconnected) +- CHECK: Nullable annotations (`?`) match actual nullability — no `!` suppression without justification +- CHECK: Early return pattern used when null check makes remaining code unreachable +- CHECK: Check if stream is seekable (`CanSeek`) before copying — use the original stream directly if seekable +- CHECK: Fall back to `Application.Current.FindMauiContext()` when `Window.MauiContext` is null +- CHECK: Initialize WebView cookies in `CoreWebView2Initialized` — preloading before `CoreWebView2` init hits null references +- CHECK: Try-catch for fire-and-forget platform calls that can fail non-critically — unhandled exceptions crash the app +- CHECK: Guard against empty collections in chained calls — `collection?.FirstOrDefault().ToPlatform()` throws if collection is empty (not null); check `.Any()` first + +### 10. Cross-Platform Behavioral Consistency `[moderate]` + +Same feature produces equivalent user-visible behavior across all target platforms. + +- CHECK: New control behavior is implemented on all platforms, not just the one that reported the bug +- CHECK: Platform-specific workarounds are documented with TODO for future alignment +- CHECK: Default property values produce the same visual result across platforms +- CHECK: Event firing order and frequency are consistent across platforms + +### 11. Memory Leak Prevention `[major]` + +Proper event unsubscription, weak references, and GC eligibility after visual tree removal. + +- CHECK: Event handlers are unsubscribed in DisconnectHandler using `-=` or nulling the delegate +- CHECK: Views and ViewModels become GC-eligible after removal from visual tree +- CHECK: Weak references are used for long-lived observers of short-lived objects +- CHECK: Memory leak tests verify collection with WeakReference pattern +- CHECK: Store handler references as `WeakReference` — a strong handler↔platform view cycle prevents collection, especially on iOS +- CHECK: Prefer delegates/`Func<>` over handler references — layout code uses `Func<>` callbacks to avoid coupling to handler instances +- CHECK: Prefer static callbacks on iOS — move gesture recognizer callbacks and event handlers into static methods, passing state through sender/tag +- CHECK: Unsubscribe Android listeners (`view.SetOnXxxListener(null)`) in DisconnectHandler — removes Java's reference that blocks GC of the handler +- CHECK: Closures capturing UIKit views (`UIView`, `UIScrollView`, `NSObject` subclasses) create hidden strong references — extract to local, use weak capture, or mark lambda `static` +- CHECK: Static `NSString` constants should be `static readonly` fields, not allocated on every use +- CHECK: CollectionView data stored via attached properties on `BindableObject` persists for the CV's lifetime — for per-item data, store on the handler instance instead +- CHECK: When adding new event subscriptions or handler references, consider adding a leak-detection device test + +#### Platform notes +- **Android**: Unsubscribe listeners in DisconnectHandler. Do NOT call Dispose on platform objects — the GC bridge handles collection. +- **iOS**: Prefer static callbacks to avoid retain cycles; remove NSNotificationCenter observers. Reference-counting GC is especially sensitive to cycles. + +### 12. Backward Compatibility and Migration `[major]` + +Breaking changes, Xamarin.Forms migration paths, and third-party renderer impact. + +- CHECK: Behavioral changes from Xamarin.Forms have explicit design justification +- CHECK: Removed/renamed APIs go through `[Obsolete]` cycle before removal +- CHECK: Default property value changes are evaluated for impact on existing apps +- CHECK: Compatibility renderers maintain behavioral parity with handler equivalents + +### 13. Platform-Specific Code Scoping `[moderate]` + +Correct `#if` guards, API-level checks, platform file extensions, and namespace conventions. + +- CHECK: `#if ANDROID`/`IOS`/`WINDOWS` guards scope code to correct compilation targets +- CHECK: Android API-level checks use `Build.VERSION.SdkInt`, not version string parsing +- CHECK: Files use correct extension (`.android.cs`, `.ios.cs`, `.windows.cs`) +- CHECK: `System.OperatingSystem` APIs used for linker-friendly runtime platform detection +- CHECK: Use type aliases for namespace collisions — `using AView = Android.Views.View;`, `using NativeScrollView = Microsoft.UI.Xaml.Controls.ScrollViewer;`, etc. +- CHECK: Platform views live in `Microsoft.Maui.Platform` namespace, under `Platform//` +- CHECK: Don't change handler generic type parameters — e.g., `ViewHandler` → `ViewHandler` is a binary breaking API change +- CHECK: When fixing behavior on one platform, verify consistency on others — reviewers will ask "What does Windows/Android do here?" +- CHECK: Reuse platform APIs via extension methods — don't duplicate logic that already exists + +#### Platform notes +- **Android**: API-level checks required for features across SDK 23–36 +- **iOS**: `.ios.cs` compiles for BOTH iOS and MacCatalyst; use `.maccatalyst.cs` for MacCatalyst-only +- **Windows**: WinUI version checks may be needed for specific Windows App SDK features + +### 14. Native Platform Defaults Preservation `[moderate]` + +Storing and restoring native defaults before applying cross-platform overrides. + +- CHECK: Native defaults are captured BEFORE any cross-platform property is applied in ConnectHandler +- CHECK: Clearing a property (setting to null/default) restores the captured native default, not a hardcoded value +- CHECK: Platform styles (WinUI XAML, iOS storyboards) are respected as defaults +- CHECK: Font resolution in compatibility renderers matches handler behavior for DefaultFont lookup + +#### Platform notes +- **Windows**: WinUI XAML styles must be preserved; clearing color restores style-applied color, not transparent + +### 15. Safe Area and Window Insets `[critical]` + +Safe area adjustments, keyboard insets, and ancestor hierarchy walks. See `safe-area-ios.instructions.md` for detailed architecture. + +- CHECK: Ancestor walk checks handle the SAME edges — parent handling Top does not block child handling Bottom +- CHECK: Safe area comparison uses pixel-level tolerance to absorb sub-pixel animation noise +- CHECK: Never gate per-view safe area callbacks on window-level insets (they diverge on MacCatalyst with custom TitleBar) +- CHECK: Safe area caches are invalidated on inset changes and window transitions +- CHECK: Only `ISafeAreaView`/`ISafeAreaView2` views receive safe area adjustments — non-safe-area views must return empty padding +- CHECK: Raw vs adjusted inset comparison — `_safeArea` is filtered by `GetSafeAreaForEdge`; raw `UIView.SafeAreaInsets` includes all edges; never compare across types +- CHECK: Use constants for magic strings — property names like `"SafeAreaInsets"` must be constants, not bare strings +- CHECK: New safe area types belong in `src/Core` so `ISafeAreaView2` can reference them — don't add core interface deps to `Controls.csproj` +- CHECK: Before creating versioned interfaces (`ISafeAreaView2`), check if the existing interface can be extended with default interface methods + +#### Platform notes +- **iOS/MacCatalyst**: See `safe-area-ios.instructions.md` for `IsParentHandlingSafeArea`, `EqualsAtPixelLevel`, and the Window Guard anti-pattern. macCatalyst defaults `UseSafeArea` to `true` (unlike iOS where it's `false`). +- **Android**: `WindowInsetsCompat` for keyboard and system bar; `fitsSystemWindows` behavior differs by API level + +### 16. Complexity Reduction `[minor]` + +Flagging overcomplicated solutions when simpler alternatives or existing infrastructure exist. + +- CHECK: Existing infrastructure is not being reimplemented (raw Task vs CancellationTokenSource pattern) +- CHECK: Predicate/filter parameters are justified — prefer direct lookup when possible +- CHECK: Abstraction layers add clear value — no wrapper classes that only delegate +- CHECK: Conditional logic can be simplified (nested if/else → single expression) + +### 17. Type Choice and Data Modeling `[moderate]` + +Correct type selection based on boxing, equality semantics, and API evolution. + +- CHECK: Struct used only when value semantics needed AND type will not be boxed frequently +- CHECK: Record types preferred over struct when value will be boxed (passed as `object`) +- CHECK: Flags enums validate that combined values are meaningful +- CHECK: Interface vs abstract class choice considers default implementations and state needs + +### 18. Trimming and AOT Compatibility `[moderate]` + +Patterns that work with .NET trimmer and NativeAOT. + +- CHECK: No `Type.GetType()`, `Activator.CreateInstance()`, or runtime reflection on trimmable types +- CHECK: `System.OperatingSystem` APIs used instead of `RuntimeInformation` for linker-friendly detection +- CHECK: XAML compilation paths produce code without runtime type resolution +- CHECK: `DynamicDependency` or `DynamicallyAccessedMembers` attributes applied when reflection is unavoidable + +### 19. CollectionView — iOS/MacCatalyst (Items2/) `[major]` + +UICollectionView-based handler. Items/ iOS code is DEPRECATED — all new iOS work targets Items2/. + +- CHECK: Changes target `Items2/` handler, NOT deprecated `Items/iOS/` +- CHECK: UICollectionView cell measurement invalidation is scoped — avoid full layout invalidation on single cell change +- CHECK: Custom UICollectionViewCell subclasses handle `MeasureInvalidated` only when MAUI controls need remeasuring +- CHECK: UICollectionViewCompositionalLayout configuration matches ItemsLayout specification +- CHECK: Don't double-copy ObjC arrays — `indexPathsForVisibleItems` already marshals via `CFArray.ArrayFromHandle`; calling `.ToArray()` on the result is wasteful +- CHECK: `MeasureFirstItem` cache must distinguish item types — grouped CV with undifferentiated cache applies GroupHeader size to all items, causing clipping +- CHECK: CollectionView inside ScrollView causes infinite layout loops on iOS due to unbounded `ContentSize` — add guards +- CHECK: Include gallery samples alongside tests for complex CV features (EmptyView, DataTemplateSelector) + +### 20. CollectionView — Android (Items/) `[major]` + +RecyclerView-based handler. This is the ONLY Android CollectionView implementation — Items2/ has NO Android code. + +- CHECK: Adapter uses range-specific notifications when INCC provides exact affected ranges; full refresh (`notifyDataSetChanged`) is valid for Reset, ambiguous indexes, and header/footer changes +- CHECK: ViewHolder recycling does not leak stale data — BindingContext updated on rebind +- CHECK: Layout manager selection (Linear, Grid, custom) matches ItemsLayout specification +- CHECK: Scroll position restoration works after adapter data changes +- CHECK: Changes go to `Items/Android/` — Items2/ has NO Android code + +### 21. CollectionView — Shared Models `[moderate]` + +Platform-independent CollectionView code: item source adapters, selection models, grouping, ItemsLayout. + +- CHECK: ObservableCollection change notifications handled correctly for Add, Remove, Replace, Move, Reset +- CHECK: Selection mode changes propagate to all platform handlers consistently +- CHECK: GroupHeaderTemplate/GroupFooterTemplate changes invalidate the correct scope +- CHECK: ItemsLayout changes trigger full handler reconfiguration, not partial updates + +### 22. Android Platform Specifics `[moderate]` + +Android-specific patterns: JNI, resources, native logging, and Glide. + +- CHECK: Store `Context` in a local before repeated use — accessing the property crosses the JNI bridge on every call +- CHECK: Android resource files have correct build action (`AndroidResource`, `EmbeddedResource`) — wrong action causes `FileNotFoundException` on Android only +- CHECK: Use `PlatformLogger` for native code logging under `src/Core/AndroidNative/`, not `android.util.Log` directly +- CHECK: When resolving Glide request managers, follow Glide's own `Activity` lookup pattern — don't add unnecessary `FragmentActivity` checks +- CHECK: New Java test projects require CI pipeline updates to execute — prefer C# device tests; if adding Java tests, include the pipeline changes + +### 23. iOS/macCatalyst Platform Specifics `[major]` + +iOS-specific patterns: reference counting, lifecycle, UIKit, and macCatalyst differences. + +- CHECK: When an iOS platform view holds a handler reference, the reference-counting GC often cannot break the cycle — use delegates, `Func<>`, or `WeakReference` patterns (see `DatePickerDelegate` proxy pattern) +- CHECK: Subscribe/unsubscribe to handler-dependent callbacks in `MovedToWindow`/removed-from-window — more reliable than constructor/dispose for iOS lifecycle +- CHECK: Store notification `UserInfo` in a local before repeated access — multiple `?.` on `notification.UserInfo` is wasteful and obscures null checks +- CHECK: Check `Handle == IntPtr.Zero` for disposed native objects — a `UICollectionView` may be disposed but not null +- CHECK: Wrap CollectionView layout callbacks in try-catch for `ObjectDisposedException`/`InvalidOperationException` — callbacks can fire after disposal +- CHECK: `UIImage.FromImage` creates a copy — if you only need to transform the existing image, modify in place when possible +- CHECK: Use `IUITextInput` interface for cursor/text range APIs across `UITextField`/`UITextView` — avoids duplicating code per concrete type + +### 24. Windows Platform Specifics `[moderate]` + +Windows/WinUI-specific patterns: Appium accessibility, WebView2, MSBuild, and theming. + +- CHECK: `BoxView` and other elements without text are invisible to Appium on WinUI — use `Label` or `AutomationProperties.Name` for elements that need test location +- CHECK: Guard null/empty collections before `.FirstOrDefault()` — on Windows, `.FirstOrDefault().ToPlatform()` on an empty `Accelerators` collection will throw +- CHECK: WebView2 assembly identity differs between WinUI, WPF, and WinForms — cannot directly share WebView2 helper code across targets +- CHECK: Prefix new MSBuild properties with `Maui` (e.g., `MauiEnableXamlLoading`) to avoid collisions with WindowsAppSdk properties +- CHECK: Apply theme per window, not to all windows — `ApplyThemeToAllWindows()` iterates redundantly (N+N-1+...+1 total for N windows) +- CHECK: Track applied state to avoid redundant theme/style work — use a per-window tracking mechanism + +### 25. Navigation & Shell `[major]` + +Shell tab switching, flyout lifecycle, and WebView URL security. + +- CHECK: Shell removes and re-adds platform views on tab switch — code that nullifies state in `DisconnectHandler` or `RemovedFromSuperview` will break on re-navigation +- CHECK: Test flyout state after window resize AND rotation as separate test methods — combined tests obscure which scenario fails +- CHECK: Split unrelated behaviors into separate tests — a test covering both "flyout-after-maximize" and "flyout-after-rotation" is actually two tests +- CHECK: Validate WebView URL mapping — local file mapping hostname should be random or scoped to prevent cross-origin file access +- CHECK: iOS "More" tab for overflow Shell items may not have standard Apple HIG behavior — verify push navigation correctness + +### 26. XAML & Bindings `[moderate]` + +Compiled bindings, source generation, and XAML compilation correctness. + +- CHECK: Compiled bindings require explicit `x:DataType` — every `{Binding}` with an explicit `Source` must have `x:DataType` on the binding or parent element; missing `x:DataType` causes runtime reflection fallback +- CHECK: Set `x:DataType` on root element when page sets `BindingContext` in code-behind +- CHECK: Don't subscribe on every `BindingContext` change in reusable views (`TemplatedCell`) — only subscribe if the BindingContext actually changed, otherwise subscriptions accumulate +- CHECK: Use `SetValue(BP, ...)` when a BindableProperty exists — source generators must use BP access, not direct property setters +- CHECK: Remove dead code from source generation — when `SkipProperties` is used, `CreateValuesVisitor` should also skip `new` instantiations for those properties +- CHECK: Markup extension recognition should be semantic — query the compilation for `IMarkupExtension` types, not just suffix matching +- CHECK: Auto-escape XML-unfriendly characters (`<`, `>`, `&&`, `||`) in XAML expression contexts — users should not need to type `>` + +### 27. Image Handling `[moderate]` + +Image source services, Glide callbacks, bitmap lifecycle, and clipping. + +- CHECK: `IImageSourceService.GetDrawableAsync` returns actual image data, not just a status — enables usage without a view (e.g., notification icons) +- CHECK: Verify Glide callback thread assumptions — `onResourceReady` is assumed to be on the main thread but this isn't documented; verify via Glide source +- CHECK: Clean up bitmaps on overlay add/remove cycles — disposal without a reload path causes blank overlays +- CHECK: Inner vs outer corner radius for clipping — `RoundRectangle` clip inside a `Border` uses the inner radius, not the outer border radius; outer value leaves visible gaps + +### 28. Gestures `[moderate]` + +Tap/click semantics, precondition verification, and span calculations. + +- CHECK: Use `Tap` over `Click` in UI tests for mobile platforms — `Click` may not convert to `Tap` on all platforms +- CHECK: Gesture tests must verify their precondition — a test for "GetPosition returns correct coordinates" must confirm the element was actually tapped; untappable elements make the test pass trivially +- CHECK: Span tap region calculation across multiple lines — the `CGRect` for each `Span` in `FormattedString` is incorrect when text wraps (inherited from Xamarin.Forms) + +### 29. Build & MSBuild `[moderate]` + +NuGet feed security, build task dependencies, feature flags, and auto-generated files. + +- CHECK: Use `dotnet-public` feed — adding arbitrary third-party feeds creates dependency confusion risk; new package sources require approval +- CHECK: Build tasks (`Controls.Build.Tasks.csproj`) cannot depend on optional NuGet packages like Maps — only core assemblies +- CHECK: Feature flag properties belong in `src/Core` (`Microsoft.Maui.dll`) — don't scatter across Controls or platform assemblies +- CHECK: Document feature switch breakage and alternatives — which APIs break when disabled, what users should do instead +- CHECK: NuGet vs workload import timing — moving MSBuild targets between them changes relative import order; test: install VS → new project → restore → build +- CHECK: Never commit `cgmanifest.json` or `templatestrings.json` — auto-generated during CI + +### 30. Accessibility `[moderate]` + +Font scaling, WinUI accessible elements, and property propagation. + +- CHECK: Don't disable font scaling globally via implicit styles — "Rather have an ugly app that a partially blind person can use instead of a beautiful one they can't" +- CHECK: Verify `AutomationProperties` propagate to the native accessibility tree — broken binding silently removes accessibility + +--- + +## What NOT to Flag + +Do not waste reviewer time on these: + +| Category | Why | +|----------|-----| +| **Style/formatting** | CI enforces via `dotnet format`. | +| **Missing XML docs on non-public APIs** | Not required by MAUI convention. | +| **Test naming preferences** | Unless names are genuinely misleading. | +| **`var` vs explicit types** | Project allows both; consistency within a file is sufficient. | +| **Micro-optimizations in cold paths** | Readability wins unless profiling proves it's a hot path. | +| **Single-use LINQ vs foreach** | Either is fine; don't bikeshed. | +| **Comment style** | Only flag if a comment is factually wrong or stale. | +| **PR commit count/squash** | That's the author's workflow choice. | + +--- + +## Dimension Routing + +Map each changed file against this table to determine which dimensions to activate. + +### Core Framework + +| Path Pattern | Dimensions | Platform | +|---|---|---| +| `src/Core/src/Layouts/**` | Layout Measure-Arrange, Performance-Critical Path, Logic and Correctness | all | +| `src/Core/src/Handlers/**` | Handler Mapper and Property Patterns, Public API Surface, Architectural Layer | all | +| `src/Core/src/Platform/Android/**` | Memory Leak Prevention, Async and Threading, Android Platform | android | +| `src/Core/src/Platform/iOS/**` | Safe Area, Performance-Critical Path, Memory Leak, iOS/MacCatalyst Platform | ios+maccatalyst | +| `src/Core/src/Platform/Windows/**` | Native Defaults Preservation, Async and Threading, Windows Platform | windows | + +### CollectionView + +| Path Pattern | Dimensions | Platform | +|---|---|---| +| `src/Controls/src/Core/Handlers/Items/Android/**`, `Items/*.Android.cs` | CollectionView Android, Performance, Memory Leak | android | +| `src/Controls/src/Core/Handlers/Items/*.Windows.cs` | CollectionView Shared Models, Native Defaults Preservation | windows | + +| `src/Controls/src/Core/Handlers/Items2/**` | CollectionView iOS/MacCatalyst, Layout, Memory Leak | ios+maccatalyst | +| `src/Controls/src/Core/Handlers/Items/iOS/**`, `Items/*.iOS.cs` | CollectionView iOS *(DEPRECATED — flag if new work)* | ios+maccatalyst | +| `src/Controls/src/Core/Items/*.cs` | CollectionView Shared Models, Backward Compatibility, Regression | all | + +### Controls — Handlers & Navigation + +| Path Pattern | Dimensions | Platform | +|---|---|---| +| `src/Controls/src/Core/Handlers/**` (non-Items) | Handler Mapper, Null Safety, Cross-Platform Consistency | all | +| `src/Controls/src/Core/Shell/**`, `src/Controls/src/Core/Handlers/Shell/**` | Navigation & Shell, Logic and Correctness, Regression Prevention | all | +| `src/Controls/src/Core/{View,Page,Layout,VisualElement,Element}/**` | Public API Surface, Architectural Layer, Backward Compatibility | all | +| `src/Controls/src/Core/*Gesture*/**` | Gestures, Logic and Correctness | all | +| `src/Controls/src/Core/Image*/**`, `src/Core/src/ImageSources/**` | Image Handling, Performance-Critical Path | all | +| Any file touching `AutomationProperties`, `SemanticProperties` | Accessibility, Cross-Platform Consistency | all | + +### XAML, Bindings & Source Generation + +| Path Pattern | Dimensions | Platform | +|---|---|---| +| `src/Controls/src/Xaml/**` | XAML & Bindings, Trimming/AOT | all | +| `src/Controls/src/BindingSourceGen/**`, `src/Controls/src/SourceGen/**` | XAML & Bindings, Trimming/AOT, Public API Surface | all | + +### Build & Engineering + +| Path Pattern | Dimensions | Platform | +|---|---|---| +| `eng/**`, `src/Controls/src/Build.Tasks/**` | Build & MSBuild, Regression Prevention | all | + +### Platform Detection + +| Extension/Directory | Platform | +|---|---| +| `*.Android.cs`, `*.android.cs`, `**/Platform/Android/**`, `**/Platforms/Android/**` | android | +| `*.iOS.cs`, `*.ios.cs`, `**/Platform/iOS/**`, `**/Platforms/iOS/**` | ios + maccatalyst | +| `*.MacCatalyst.cs`, `*.maccatalyst.cs`, `**/Platform/MacCatalyst/**`, `**/Platforms/MacCatalyst/**` | maccatalyst only | +| `*.Windows.cs`, `*.windows.cs`, `**/Platform/Windows/**`, `**/Platforms/Windows/**` | windows | + +### Always-Active Dimensions + +These apply regardless of file paths: Logic and Correctness, Regression Prevention, Complexity Reduction. + +### Conditional Dimensions + +| Dimension | Trigger | +|---|---| +| Public API Surface | Adds/removes `public` members or modifies `PublicAPI.Unshipped.txt` | +| Trimming/AOT | Uses reflection, `Type.GetType`, or `Activator.CreateInstance` | +| Backward Compatibility | Changes defaults, removes APIs, or touches Compatibility/ | + +--- + +## Review Workflow + +### Wave 0 — Build Briefing Pack + +1. Read PR diff (`gh pr diff`) and list changed files — form your own assessment BEFORE reading PR description (independence-first) +2. Map changed files to dimensions using the routing table above +3. Identify affected platforms from file paths using the platform detection table above +4. THEN read the PR description and linked issues for design intent — compare with your independent assessment +5. Read existing PR review comments to identify feedback already given — avoid duplicating +6. If a changed file does not map to any dimension, still scan it for Principles 1–8 + +### Wave 1 — Find (parallel sub-agents, batches of 6) + +For each activated dimension, launch a sub-agent. The sub-agent: +1. Walks every changed hunk relevant to that dimension +2. Evaluates each CHECK rule against the diff +3. **Every finding MUST have a file path.** Try hard to associate to a specific line. Priority order: + - `file:exact_line` — the specific line where the issue manifests (strongly preferred) + - `file:1` — when the issue is about the file but no single line captures it (e.g., missing import, structural concern) + - Text fallback — **only** when the finding genuinely cannot be associated with any file in the diff (e.g., "this PR is missing tests entirely"). This is the worst-case fallback, not a convenience option. +4. Appends findings to the configured findings JSON (default `inline-findings.json`). Only returns text to the top-level agent if file association truly failed. +5. Returns a count: "N inline findings written" (and if any text fallbacks: "M could not be placed on a file") + +**Threshold**: only record findings with a concrete failing scenario. Stylistic preferences are not findings. + +Run sub-agents in parallel batches of 6 dimensions at a time. + +### Wave 2 — Validate (prove or disprove each finding) + +For each potential finding from Wave 1: +1. Read surrounding context (not just the diff hunk) to check if the issue is already handled +2. Check if tests in the PR cover the scenario +3. Check git blame to see if the pattern is intentional +4. Discard findings that cannot survive validation — false positives erode trust + +**Severity assignment**: +- `critical` — data loss, crash, infinite loop, security issue +- `major` — incorrect behavior visible to users, memory leak, performance regression on hot path +- `moderate` — suboptimal pattern, missing edge case, API design concern +- `minor` — style, simplification opportunity, documentation gap + +### Wave 3 — Record and Post Findings + +**Always write the findings file** — every finding that can be associated with a file+line goes here. Try hard to associate feedback to a specific location. + +**Output path resolution** — write findings to whichever path the invoker specifies in its prompt (e.g. `OUTPUT_FINDINGS_PATH=...`, `outputPath: ...`, or any equivalent explicit instruction). If the invoker does not specify a path, default to `CustomAgentLogsTmp/PRState/{PR}/PRAgent/inline-findings.json`. This lets internal callers (e.g. `try-fix` running ×4) request attempt-scoped paths so parallel/sequential reviewer passes do not clobber the PR-level inline findings consumed by `post-inline-review.ps1`. + +```json +[ + { + "path": "src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs", + "line": 42, + "body": "**[major] Layout Measure-Arrange** — Content measured with unconstrained height but arranged with bounded height. Concrete scenario: ScrollView inside a Grid with Star row height." + } +] +``` + +Each entry has exactly 3 fields matching the GitHub Pull Request Review API: +- **`path`** (string) — file relative to repo root, must exist in the PR diff +- **`line`** (integer ≥ 1) — line number **in the file on the PR branch** (right side of the diff). Must be a line that appears in the diff — the GitHub API rejects lines not in the diff with a 422 error. Use line 1 only as a fallback for file-level concerns. +- **`body`** (string) — the comment text. Embed severity and dimension in the text: `**[severity] Dimension** — description` + +Rules: +- Group related findings on adjacent lines into a single entry +- Limit to ≤15 findings — prioritize by severity +- Exclude findings already present in existing PR comments (checked in Wave 0 step 5) + +**After writing, validate the JSON.** Read back the file, verify it parses as a JSON array, and check every entry has `path` (string), `line` (integer ≥ 1), and `body` (string). If validation fails, fix the file and re-validate. + +**Text fallback** — absolute last resort, only when no file in the diff can carry the finding. If you can name even one file the concern applies to, use `file:1` instead. + +The sole output of this agent is the JSON findings file (at the invoker-specified or default path). There is no text return, no `.md` output, no dimension summary table. Each finding carries its dimension and severity in the `body` field — that is the complete record. + +--- + +## Operational Notes + +- **CollectionView handler detection**: Items/ is the active handler for Android+Windows. Items2/ is the active handler for iOS/MacCatalyst. Items/ also contains deprecated iOS files (`*.iOS.cs`) — only modify those for legacy maintenance. Never suggest "also fix in Items2/" for Android code or vice versa. See `collectionview-handler-detection.instructions.md` for full mapping. +- **File extension semantics**: Both lowercase and PascalCase forms exist (`.ios.cs`/`.iOS.cs`). The iOS form compiles for both iOS AND MacCatalyst. The MacCatalyst form compiles for MacCatalyst only. Both compile for MacCatalyst builds when both exist. diff --git a/.github/instructions/collectionview-android.instructions.md b/.github/instructions/collectionview-android.instructions.md new file mode 100644 index 000000000000..1c7e53e88636 --- /dev/null +++ b/.github/instructions/collectionview-android.instructions.md @@ -0,0 +1,32 @@ +--- +applyTo: + - "src/Controls/src/Core/Handlers/Items/Android/**" + - "src/Controls/src/Core/Handlers/Items/*.Android.cs" + - "src/Controls/src/Core/Handlers/Items/*.android.cs" +--- +# CollectionView — Android (Items/ Handler) + +> Items/ is the sole Android handler — see `collectionview-handler-detection.instructions.md` for the full platform→handler mapping. + +## RecyclerView Adapter Patterns +- Use range-specific notifications (`NotifyItemRangeInserted`, `NotifyItemRangeRemoved`, `NotifyItemRangeChanged`) when INCC semantics provide exact affected ranges — prefer these over `NotifyDataSetChanged` for preserving scroll position and animations +- Full refresh via `NotifyDataSetChanged` is valid for `Reset` actions, ambiguous index cases, and header/footer template changes +- Handle all `ObservableCollection` change actions: Add, Remove, Replace, Move, Reset +- On `Reset`, recalculate adapter state from scratch — do not assume incremental consistency + +## ViewHolder Recycling +- `BindingContext` MUST be updated on every rebind — stale data from a previous holder is a common source of visual glitches +- Do not store item-specific state in the ViewHolder outside of `BindViewHolder` — recycled holders carry state from previous items +- Dispose and recreate platform views only when the template changes, not on every rebind + +## Layout Manager +- Select layout manager (Linear, Grid, custom) based on `ItemsLayout` specification — do not hardcode +- `ItemsLayout` property changes require full layout manager replacement, not partial reconfiguration +- Account for Android pixel rounding in item measurement — fractional dp values cause 1px gaps + +## Memory and Lifecycle +- Unsubscribe from `ScrollChange` and adapter observers in `DisconnectHandler` — do NOT call Dispose on platform objects +- Scroll position restoration after adapter data changes must handle empty-collection edge case + +## Regression Patterns +- Test across empty collection, single item, many items, and with grouping — a fix for one layout scenario routinely breaks another diff --git a/.github/instructions/collectionview-ios.instructions.md b/.github/instructions/collectionview-ios.instructions.md new file mode 100644 index 000000000000..3c7ac2911020 --- /dev/null +++ b/.github/instructions/collectionview-ios.instructions.md @@ -0,0 +1,34 @@ +--- +applyTo: + - "src/Controls/src/Core/Handlers/Items2/**" + - "src/Controls/src/Core/Handlers/Items/iOS/**" + - "src/Controls/src/Core/Handlers/Items/*.iOS.cs" + - "src/Controls/src/Core/Handlers/Items/*.ios.cs" +--- +# CollectionView — iOS/MacCatalyst (Items2/ Handler) + +> New iOS/MacCatalyst work targets Items2/. Items/iOS/ is deprecated. See `collectionview-handler-detection.instructions.md` for the full platform→handler mapping. + +## UICollectionView Cell Measurement +- Scope cell layout invalidation to the affected cell — avoid `InvalidateLayout()` on the entire collection for single-cell changes +- Custom `UICollectionViewCell` subclasses should handle `MeasureInvalidated` only when the hosted MAUI control actually needs remeasuring +- Cell sizing must stay in sync between the measure pass and the layout pass — out-of-sync causes visual glitches + +## UICollectionViewCompositionalLayout +- Layout configuration must match the `ItemsLayout` specification (Linear, Grid, or custom) +- `ItemsLayout` property changes require full layout reconfiguration — partial updates leave stale section configuration +- Group header/footer template changes must invalidate the correct section scope, not the entire layout + +## Memory Management +- Use static callback patterns to avoid retain cycles between cells and their hosting handler +- Remove `NSNotificationCenter` observers in `DisconnectHandler` +- Weak references for long-lived observers of short-lived cells — cells are recycled and reused + +## Regression Patterns +- Test across empty collection, single item, many items, and with grouping — a fix for one layout scenario routinely breaks another + +## Items/ iOS (Deprecated) +- Files in `Handlers/Items/*.iOS.cs` are deprecated — prefer Items2/ for new work +- Only modify Items/ iOS code for explicit legacy maintenance or backward-compatibility fixes + +> **Platform file extension rules** (`.ios.cs` vs `.maccatalyst.cs` compilation targets) are defined in `copilot-instructions.md` § Platform-Specific File Extensions. See also `collectionview-handler-detection.instructions.md` for which handler directory to target per platform. diff --git a/.github/instructions/collectionview-windows.instructions.md b/.github/instructions/collectionview-windows.instructions.md new file mode 100644 index 000000000000..a9eff1612549 --- /dev/null +++ b/.github/instructions/collectionview-windows.instructions.md @@ -0,0 +1,26 @@ +--- +applyTo: + - "src/Controls/src/Core/Handlers/Items/*.Windows.cs" + - "src/Controls/src/Core/Handlers/Items/*.windows.cs" +--- +# CollectionView — Windows (Items/ Handler) + +Items/ is the **ONLY** Windows CollectionView implementation. Items2/ has NO Windows code. + +## WinUI ListView/ItemsRepeater Patterns +- Preserve WinUI XAML styles applied via native theming — clearing a MAUI property must restore the style-applied value, not a hardcoded default +- `double.NaN` is the WinUI convention for unconstrained dimensions — do not confuse with MAUI's `double.PositiveInfinity` +- Use `DispatcherQueue.TryEnqueue` for deferred UI thread work — do not use `Dispatcher.BeginInvoke` + +## Data Source and Change Notifications +- Handle all `ObservableCollection` change actions (Add, Remove, Replace, Move, Reset) with range-scoped updates +- Avoid full source refresh (`NotifyDataSetChanged` equivalent) — it kills selection state and scroll position +- Selection mode changes must propagate correctly to the native `SelectionMode` property + +## Layout Configuration +- `ItemsLayout` changes require reconfiguration of the underlying panel (e.g., `ItemsWrapGrid`, `ItemsStackPanel`) +- Verify that `ItemsLayout.Span` for grid layouts maps correctly to WinUI's `MaximumRowsOrColumns` + +## Cross-Platform Consistency +- Default values for control properties must produce the same visual result as Android and iOS +- Event firing order (selection changed, scrolled) should match other platforms for the same user interaction diff --git a/.github/instructions/handler-patterns.instructions.md b/.github/instructions/handler-patterns.instructions.md new file mode 100644 index 000000000000..986d9d8e8e9c --- /dev/null +++ b/.github/instructions/handler-patterns.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: + - "src/Core/src/Handlers/**" + - "src/Controls/src/Core/Handlers/**" +--- +# Handler Mapper and Property Patterns + +## Property Update Flow +- Property updates MUST go through `Handler.UpdateValue(nameof(Property))` — never call mapper methods directly +- Direct calls bypass user-registered `AppendToMapping`/`PrependToMapping` customizations +- Map dependencies before dependents — if property B reads property A, A's mapper must run first +- `CommandMapper` entries return void and use the `(handler, view, args)` signature + +## Lifecycle Management +- **ConnectHandler**: Register listeners, subscribe to events, capture native defaults BEFORE applying cross-platform properties +- **DisconnectHandler**: Unsubscribe all events, dispose platform resources, null out references +- Call `base.ConnectHandler`/`base.DisconnectHandler` — base class performs platform hookup/teardown (NOT mapper initialization, which happens in `SetVirtualView`) + +## Null Safety in Callbacks +- Null-check `VirtualView` before access in every mapper method and platform callback — it is null during disconnect +- Validate `MauiContext` before use — throw `InvalidOperationException` with descriptive message if null +- After async operations, verify the handler is still connected before applying results + +## Native Defaults Preservation +- Capture native default values (colors, fonts, styles) in `ConnectHandler` BEFORE any cross-platform property is applied +- Clearing a cross-platform property (setting to null/default) must restore the captured native default, not a hardcoded fallback +- On Windows, preserve WinUI XAML style-applied values; on Android, cache theme-inherited drawables; on iOS, capture UIAppearance defaults + +## Mapper Extensibility +- When extending existing mappers, ensure the base mapper chain is called — do not silently replace +- Static mapper methods should be `public static` to enable platform-specific overrides +- Use `nameof()` for property keys — avoid magic strings + +## Fire-and-Forget Async +- Use `.FireAndForget(handler)` for async operations in mapper methods — never use bare `async void` +- The `FireAndForget` overload accepting a handler logs exceptions through the handler's service provider diff --git a/.github/instructions/layout-system.instructions.md b/.github/instructions/layout-system.instructions.md new file mode 100644 index 000000000000..6959fd540f3a --- /dev/null +++ b/.github/instructions/layout-system.instructions.md @@ -0,0 +1,30 @@ +--- +applyTo: + - "src/Core/src/Layouts/**" + - "src/Controls/src/Core/Layout/**" + - "src/Core/src/Handlers/Layout/**" +--- +# Layout System Rules + +## Measure/Arrange Contract +- `Measure(widthConstraint, heightConstraint)` returns the desired size — it must not mutate layout state or trigger side effects +- `ArrangeChildren(bounds)` positions children within the given bounds — it must respect the size returned from `Measure`, not compute independently +- A child measured with `widthConstraint=200` must not be arranged with `width=300` — constraints must stay consistent across passes +- Subtract padding, margin, and border thickness from constraints BEFORE passing to children + +## Constraint Propagation +- Stack layouts pass `double.PositiveInfinity` along the stacking axis and the parent constraint along the cross axis +- Grid cells compute per-cell constraints from column/row definitions — do not pass the full grid constraint to each child +- `MeasureContent` helpers on `IContentView` handle inset subtraction — use them instead of manual arithmetic +- On Windows, `double.NaN` represents unconstrained dimensions (WinUI convention) — do not confuse with `double.PositiveInfinity` (MAUI convention) + +## Infinite Loop Avoidance +- Never create circular dependencies where child size depends on parent AND parent size depends on child +- Layout invalidation (`InvalidateMeasure`) must not re-trigger during an active measure pass +- ScrollView content must always be re-measured on layout trigger — do not aggressively cache child measurements in scrollable containers + +## Performance on Hot Paths + +> Layout measure/arrange methods are hot paths. All rules from `performance-hotpaths.instructions.md` apply — no LINQ, closures, or allocations. Additionally for layout: +- Cache expensive computations (e.g., `GridStructure`) and invalidate only when inputs change +- Bindable property change handlers should skip layout invalidation when the value has not actually changed diff --git a/.github/instructions/performance-hotpaths.instructions.md b/.github/instructions/performance-hotpaths.instructions.md new file mode 100644 index 000000000000..11632fb978b1 --- /dev/null +++ b/.github/instructions/performance-hotpaths.instructions.md @@ -0,0 +1,34 @@ +--- +applyTo: + - "src/Core/src/Layouts/**" + - "src/Core/src/Platform/**" + - "src/Controls/src/Core/Handlers/Items/**" + - "src/Controls/src/Core/Handlers/Items2/**" + - "src/Controls/src/Core/ScrollView/**" +--- +# Performance-Critical Path Rules + +> **Scope rationale**: globs target the actual hot paths — layout measure/arrange, +> platform scroll/touch callbacks, CollectionView/CarouselView (Items + Items2) +> recycling, and ScrollView. The full handler tree is intentionally NOT included; +> most handlers (Button, DatePicker, CheckBox, …) are not hot paths and don't need +> these constraints. If you're working on a handler that IS allocation-sensitive +> (image decoding, animation tick, etc.), apply these rules anyway. + +## Hot Paths in MAUI +Measure/arrange cycles, scrolling callbacks, binding propagation, and property change notifications are called at high frequency. All rules below apply to code on these paths. + +## Allocation Avoidance +- No LINQ methods (`.Where`, `.Select`, `.FirstOrDefault`, `.ToList`) — use indexed `for` loops +- No closures or lambdas that capture variables — these allocate a compiler-generated class per invocation +- No string concatenation or interpolation — use `StringBuilder` or pre-allocated strings if logging is required +- Prefer `Count` + indexer over `IEnumerable` iteration to avoid enumerator allocation + +## Caching and Invalidation +- Cache results of expensive computations called multiple times per layout pass +- Invalidate caches when inputs change — stale caches cause incorrect layout +- Skip redundant work: if a property's new value equals the old value, do not trigger layout invalidation or re-render + +## Collection Iteration +- When the source implements `IList` or `IReadOnlyList`, use `for (int i = 0; i < list.Count; i++)` instead of `foreach` +- `ObservableCollection` change handlers should process only the affected range, not re-enumerate the entire collection diff --git a/.github/instructions/public-api.instructions.md b/.github/instructions/public-api.instructions.md new file mode 100644 index 000000000000..3edd33ba70d0 --- /dev/null +++ b/.github/instructions/public-api.instructions.md @@ -0,0 +1,36 @@ +--- +applyTo: + - "**/PublicAPI.Unshipped.txt" + - "src/Core/src/**/*.cs" + - "src/Controls/src/**/*.cs" + - "src/Essentials/src/**/*.cs" +--- +# Public API Surface Design + +> **Activation guard**: only apply this guidance when the diff actually changes +> public API surface — `public`/`protected` types or members, interface +> definitions, builder/extension methods on public types, `[Obsolete]` markers, +> or `PublicAPI.*.txt` entries. The broad `.cs` globs exist so this guidance +> loads at the moment of API design (writing `public class Foo` in `Button.cs`), +> not just when the analyzer-generated `Unshipped.txt` is updated afterward. +> If the diff only changes internals or implementation details, ignore. + +## API Addition Rules +- New public APIs must have clear, demonstrated use cases — no speculative additions +- API naming must follow .NET design guidelines and be consistent with existing MAUI patterns +- Interfaces belong at `IView`/`IElement` level when behavior is cross-cutting — do not duplicate per-control + +## PublicAPI.Unshipped.txt +- Entries must exactly match the actual API shape (namespace, type, member signature) + +> For PublicAPI.Unshipped.txt file management workflow (never disable analyzers, `dotnet format analyzers`, revert-then-add pattern), see `copilot-instructions.md` § PublicAPI.Unshipped.txt File Management. + +## Obsolescence and Removal +- Deprecated APIs must go through `[Obsolete("message")]` with migration guidance before removal +- Include the replacement API or pattern in the obsolete message +- Breaking changes require explicit design justification documented in the PR + +## Visibility Decisions +- Default to `internal` — make `public` only when external consumers need access +- `protected` members in unsealed types are part of the public API surface — treat with same rigor +- Use `[EditorBrowsable(EditorBrowsableState.Never)]` for APIs that must be public for technical reasons but should not appear in IntelliSense diff --git a/.github/instructions/threading-async.instructions.md b/.github/instructions/threading-async.instructions.md new file mode 100644 index 000000000000..b9aeeb0282ce --- /dev/null +++ b/.github/instructions/threading-async.instructions.md @@ -0,0 +1,34 @@ +--- +applyTo: + - "**/*.Android.cs" + - "**/*.android.cs" + - "**/*.iOS.cs" + - "**/*.ios.cs" + - "**/*.Windows.cs" + - "**/*.windows.cs" + - "**/Platform/**" + - "**/Platforms/**" +--- +# Threading and Async Patterns + +## UI Thread Dispatch +- Platform view modifications MUST happen on the UI thread +- **Android**: `platformView.Post(() => { })` or `Looper.MainLooper` for thread checks +- **iOS/MacCatalyst**: `MainThread.BeginInvokeOnMainThread()` or `DispatchQueue.MainQueue` +- **Windows**: `DispatcherQueue.TryEnqueue()` — be aware of COM threading apartment model + +## Async Handler Operations +- Use `.FireAndForget(handler)` for async work in mapper methods — never bare `async void` +- After `await`, verify the handler is still connected (`VirtualView != null`) before applying results +- Thread `CancellationToken` through long-running operations and check at appropriate yield points + +## Race Condition Prevention +- Concurrent access to shared state must use `lock`, `Interlocked`, or immutable patterns +- Image loading and other async pipelines must cancel in-flight operations when the source changes +- Guard against stale callbacks — platform callbacks may fire after `DisconnectHandler` + +## Platform-Specific Patterns +- **Android**: API-level checks use `Build.VERSION.SdkInt` comparison, not version string parsing +- **iOS**: Use `System.OperatingSystem.IsIOSVersionAtLeast()` for linker-friendly runtime checks +- **Windows**: WinUI version checks may be needed for features in specific Windows App SDK versions +- Use `System.OperatingSystem` APIs over `RuntimeInformation` — they are trimmer/AOT-friendly diff --git a/.github/scripts/Review-PR.ps1 b/.github/scripts/Review-PR.ps1 index acd7de121f0e..50b7d622e6b9 100644 --- a/.github/scripts/Review-PR.ps1 +++ b/.github/scripts/Review-PR.ps1 @@ -7,7 +7,8 @@ Step 0: Branch setup - Create review branch from main, merge PR squashed Step 1: Gate - Run test verification directly (verify-tests-fail.ps1) - Step 2: pr-review skill - 3-phase review (Pre-Flight, Try-Fix, Report) + Step 2: Multi-candidate review - Pre-Flight, then PARALLEL (expert-reviewer eval of PR + Try-Fix×4), + then Report compares all candidates and writes winner.json Step 3: Post AI Summary - Directly runs posting scripts Step 4: Apply labels - Apply agent labels based on review results @@ -298,7 +299,10 @@ function Invoke-CopilotStep { } # Use JSON output format to stream live progress of agent activity. - & copilot -p $Prompt --allow-all --output-format json 2>&1 | ForEach-Object { + # Model is overridable via $env:COPILOT_REVIEW_MODEL so contributors without internal-model access + # can run this script (e.g., with 'claude-opus-4.6' or 'claude-sonnet-4.6'). + $copilotModel = if ($env:COPILOT_REVIEW_MODEL) { $env:COPILOT_REVIEW_MODEL } else { 'claude-opus-4.7-1m-internal' } + & copilot -p $Prompt --allow-all --output-format json --model $copilotModel 2>&1 | ForEach-Object { $line = $_.ToString() try { $event = $line | ConvertFrom-Json -ErrorAction Stop @@ -391,13 +395,13 @@ function Invoke-CopilotStep { } } 'result' { - # Final stats - $usage = $event.data.usage + # Final stats — note: 'result' is a top-level event with no 'data' wrapper. + $usage = $event.usage if ($usage) { $elapsed = $stopwatch.Elapsed.ToString("mm\:ss") $apiMs = if ($usage.totalApiDurationMs) { [math]::Round($usage.totalApiDurationMs / 1000, 1) } else { "?" } $changes = $usage.codeChanges - $filesChanged = if ($changes -and $changes.filesModified) { $changes.filesModified.Count } else { 0 } + $filesChanged = if ($changes -and $changes.filesModified) { @($changes.filesModified).Count } else { 0 } $linesAdded = if ($changes) { $changes.linesAdded } else { 0 } $linesRemoved = if ($changes) { $changes.linesRemoved } else { 0 } @@ -547,7 +551,12 @@ for ($gateAttempt = 1; $gateAttempt -le $maxGateAttempts; $gateAttempt++) { } # Clear previous attempt's report so a crash mid-run doesn't leak its classification into this one. Remove-Item $gateContentFile -Force -ErrorAction SilentlyContinue - $gateOutput = & pwsh -NoProfile -File "$verifyScript" -Platform $gatePlatform -PRNumber $PRNumber -RequireFullVerification 2>&1 + # Note: -RequireFullVerification is intentionally OMITTED. The verify script + # auto-detects fix files from the diff; if there are none (e.g., a test-only + # PR like a regression repro), it falls back to "verify failure only" mode + # and reports whether the new tests fail without any fix. Passing the flag + # would force the script to error out for those PRs. + $gateOutput = & pwsh -NoProfile -File "$verifyScript" -Platform $gatePlatform -PRNumber $PRNumber 2>&1 $gateExitCode = $LASTEXITCODE $gateOutput | ForEach-Object { Write-Host " $_" } @@ -600,7 +609,74 @@ Write-Host " 📁 Gate result: $gateResult" -ForegroundColor $gateColor # Copy the verification report to gate/content.md (always overwrite — the report is the source of truth) $verificationReport = Join-Path $gateOutputDir "verify-tests-fail/verification-report.md" # Capture last meaningful lines from gate output for fallback diagnostics -$gateLogTail = @($gateOutput | ForEach-Object { $_.ToString() } | Where-Object { $_ -match '\S' } | Select-Object -Last 20) -join "`n" +$gateLogTail = @($gateOutput | ForEach-Object { $_.ToString() } | Where-Object { $_ -match '\S' } | Select-Object -Last 60) -join "`n" + +# ─── Improvement #1: build rich fallback diagnostics for the silent-failure case ─── +# When verify-tests-fail.ps1 aborts before writing its report, the original fallback +# emitted just "Gate Result: FAILED" + an empty
block. Capture extra signal +# so reviewers and downstream agents can act on it. +function Get-GateFallbackDetails { + param([string]$Tail, [int]$ExitCode, [string]$VerifyDir, [string]$ReviewedPlatform) + + $sections = @() + + $sections += "**Exit code:** ``$ExitCode``" + + # Surface auto-detected tests / fix files from the verify script's stdout + # so reviewers can tell whether detection failed vs. the test run itself. + $detected = @{} + foreach ($line in ($Tail -split "`n")) { + if ($line -match '^\s*Detected test type:\s*(\S+)') { $detected['type'] = $Matches[1] } + elseif ($line -match '^\s*Test filter:\s*(\S+)') { $detected['filter'] = $Matches[1] } + elseif ($line -match '^\s*Fix files \((\d+)\):') { $detected['fixCount'] = $Matches[1] } + elseif ($line -match '^\s*Merge base:\s*(\S+)') { $detected['mergeBase'] = $Matches[1] } + } + if ($detected.Count -gt 0) { + $items = @() + if ($detected.ContainsKey('type')) { $items += "- Test type: ``$($detected['type'])``" } + if ($detected.ContainsKey('filter')) { $items += "- Test filter: ``$($detected['filter'])``" } + if ($detected.ContainsKey('fixCount')) { $items += "- Fix files detected: $($detected['fixCount'])" } + if ($detected.ContainsKey('mergeBase')) { $items += "- Merge base: ``$($detected['mergeBase'])``" } + $sections += "**Auto-detected:**`n" + ($items -join "`n") + } + + # Heuristic classification — make the cause actionable instead of leaving the + # reader to grep stderr. + $likely = @() + if ($Tail -match '(?i)Could not auto-detect PR number|no tests detected|0 test\(s\) detected') { + $likely += "Test detection failed — no runnable tests were found in the PR diff." + } + # Match coded build errors (CS, MSB, NU, MAUI, NETSDK, XA, etc.) without false-positiving + # on lines like "MSBUILD output: 0 error(s)". The general form `error ` covers + # compiler, MSBuild, NuGet restore, MAUI analyzer, .NET SDK, Android packaging diagnostics. + if ($Tail -match '(?i)build failed|\berror\s+[A-Z]{2,}\d+\b') { + $likely += "Build error before any test could run." + } + if ($Tail -match '(?i)emulator.*(?:timeout|failed|not.found)|adb.*(?:server|crashed)|xharness.*(?:failed|timeout)') { + $likely += "Device/emulator setup failed (env error class)." + } + if ($Tail -match '(?i)merge.conflict|conflict.*merge.base') { + $likely += "Merge conflict prevented running the gate." + } + if ($Tail -match '(?i)NO FIX FILES DETECTED') { + $likely += "No fix files detected in the diff (PR may be test-only — should now run in failure-only mode)." + } + if ($likely.Count -gt 0) { + $sections += "**Likely cause:**`n" + (($likely | ForEach-Object { "- $_" }) -join "`n") + } + + # List artifacts under gate/verify-tests-fail/ — partial logs sometimes survive + # even when the report itself was not written. + if (Test-Path $VerifyDir) { + $files = Get-ChildItem -Path $VerifyDir -File -ErrorAction SilentlyContinue | + Sort-Object Name | ForEach-Object { "- ``$($_.Name)`` ($([math]::Round($_.Length / 1KB, 1)) KB)" } + if ($files) { + $sections += "**Artifacts written before exit:**`n" + ($files -join "`n") + } + } + + return ($sections -join "`n`n") +} if (Test-Path $verificationReport) { $reportContent = Get-Content $verificationReport -Raw -ErrorAction SilentlyContinue @@ -612,13 +688,18 @@ if (Test-Path $verificationReport) { # Report exists but has bad format — generate fallback with logs Write-Host " ⚠️ Verification report has invalid format — using fallback" -ForegroundColor Yellow $resultIcon = switch ($gateResult) { "PASSED" { "✅" } "SKIPPED" { "⚠️" } default { "❌" } } + $fallbackDetails = Get-GateFallbackDetails -Tail $gateLogTail -ExitCode $gateExitCode -VerifyDir (Join-Path $gateOutputDir "verify-tests-fail") -ReviewedPlatform $gatePlatform @" ### Gate Result: $resultIcon $gateResult **Platform:** $($gatePlatform.ToUpper()) +> ⚠️ ``verify-tests-fail.ps1`` produced an empty report. Diagnostics below. + +$fallbackDetails +
-Gate output log +Gate output log (last 60 lines) `````` $gateLogTail @@ -638,13 +719,18 @@ No tests were detected in this PR. "@ | Set-Content (Join-Path $gateOutputDir "content.md") -Encoding UTF8 } else { $resultIcon = switch ($gateResult) { "PASSED" { "✅" } default { "❌" } } + $fallbackDetails = Get-GateFallbackDetails -Tail $gateLogTail -ExitCode $gateExitCode -VerifyDir (Join-Path $gateOutputDir "verify-tests-fail") -ReviewedPlatform $gatePlatform @" ### Gate Result: $resultIcon $gateResult **Platform:** $($gatePlatform.ToUpper()) +> ⚠️ ``verify-tests-fail.ps1`` exited before writing a verification report. Diagnostics below. + +$fallbackDetails +
-Gate output log +Gate output log (last 60 lines) `````` $gateLogTail @@ -655,11 +741,8 @@ $gateLogTail } } -# Post gate result by updating (or creating) the unified AI Summary comment. -# The same script is called again in STEP 3 once the review phases finish; here -# it runs early so the PR author sees the gate outcome without waiting for the -# full review. -$postGateScript = Join-Path $PSScriptRoot "post-ai-summary-comment.ps1" +# Post gate result as a separate PR comment +$postGateScript = Join-Path $PSScriptRoot "post-gate-comment.ps1" if (Test-Path $postGateScript) { try { if ($DryRun) { @@ -668,10 +751,10 @@ if (Test-Path $postGateScript) { & $postGateScript -PRNumber $PRNumber } } catch { - Write-Host " ⚠️ Failed to post gate section (non-fatal): $_" -ForegroundColor Yellow + Write-Host " ⚠️ Failed to post gate comment (non-fatal): $_" -ForegroundColor Yellow } } else { - Write-Host " ⚠️ post-ai-summary-comment.ps1 not found" -ForegroundColor Yellow + Write-Host " ⚠️ post-gate-comment.ps1 not found" -ForegroundColor Yellow } # Apply gate result label @@ -715,7 +798,49 @@ $gateStatusForPrompt = switch ($gateResult) { } $step2Prompt = @" -Use a skill to review PR #$PRNumber. +Run a multi-candidate PR review for PR #$PRNumber using the following flow. + +## Phase 1 — Pre-Flight (context only) +Use the pr-review skill's pre-flight phase to gather context. Do NOT modify code. +Write summary to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/pre-flight/content.md``. + +## Phase 2 — Candidate generation (run BOTH branches; do not skip either) +Generate the following candidates. Each candidate is an alternative diff against the PR's base branch. Do this work in isolated worktrees / scratch copies so artifacts do NOT clobber each other. + +### Branch A — Expert reviewer evaluation of the current PR fix (in sandbox) +Use the code-review skill with the maui-expert-reviewer agent to evaluate the PR's existing fix. Apply the reviewer's actionable feedback in a sandbox copy and treat the result as a candidate named ``pr-plus-reviewer``. +- Always also write the raw inline findings to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json`` (these are file:line findings against the PR's diff and feed the inline-comment posting step). +- Write candidate output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/expert-pr-eval/content.md``. + +### Branch B — Try-Fix ×4 (ALWAYS runs — do NOT skip) +Use the pr-review skill's try-fix phase to generate FOUR independent candidate fixes (``try-fix-1`` through ``try-fix-4``). Each candidate must load domain knowledge from a different maui-expert-reviewer dimension so the candidates are diverse. +- 🚨 You MUST generate all four candidates. Do not short-circuit even if Pre-Flight or the expert eval suggests the PR is already correct. +- Write each candidate's output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/try-fix-{N}/content.md`` (N = 1..4). +- Aggregate try-fix narrative for the AI summary comment to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/try-fix/content.md``. + +## Phase 3 — Report +The expert reviewer evaluates ALL candidates against each other: +- ``pr`` (the raw PR fix as submitted) +- ``pr-plus-reviewer`` (PR fix + reviewer feedback applied in sandbox) +- ``try-fix-1``..``try-fix-4`` +Pick the single winning candidate. +Write the comparative analysis to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/report/content.md``. + +## Phase 4 — Winner manifest (REQUIRED) +Write ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/winner.json`` with this exact schema: +``````json +{ + "schemaVersion": 1, + "winner": "pr" | "pr-plus-reviewer" | "try-fix-1" | "try-fix-2" | "try-fix-3" | "try-fix-4", + "isPRFix": true | false, + "summary": "1-3 sentence rationale for why this candidate won", + "candidateDiff": "" +} +`````` +Rules: +- ``isPRFix`` MUST be ``true`` when ``winner`` is ``pr`` or ``pr-plus-reviewer``. +- ``isPRFix`` MUST be ``false`` when ``winner`` is any ``try-fix-*``. +- When ``isPRFix`` is ``false``, ``candidateDiff`` MUST be a non-empty unified diff. Truncate at 55 KB if larger and end with a ``... [truncated]`` marker line. $platformInstruction $autonomousRules @@ -723,9 +848,6 @@ $autonomousRules **Gate result (already completed in a prior step):** $gateStatusForPrompt Do NOT re-run gate verification. The gate phase is handled separately. ⚠️ Do NOT create or overwrite ``gate/content.md`` — it is already generated by the gate script with detailed test output. - -📁 Write phase output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/{phase}/content.md`` -(phases: pre-flight, try-fix, report — NOT gate) "@ Invoke-CopilotStep -StepName "STEP 2: PR REVIEW" -Prompt $step2Prompt | Out-Null @@ -806,6 +928,138 @@ if (Test-Path $reviewScript) { Write-Host " ⚠️ post-ai-summary-comment.ps1 not found — skipping review summary" -ForegroundColor Yellow } +# Determine winning candidate (winner.json) — drives whether we post inline findings or request changes +$winnerFile = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/winner.json" +$winner = $null +if (Test-Path $winnerFile) { + try { + $winner = Get-Content -Raw -LiteralPath $winnerFile | ConvertFrom-Json + # Validation + $allowed = @('pr','pr-plus-reviewer','try-fix-1','try-fix-2','try-fix-3','try-fix-4') + if (-not $winner.winner -or $allowed -notcontains $winner.winner) { + Write-Host " ⚠️ winner.json has invalid 'winner' value: $($winner.winner) — falling back to PR-fix path" -ForegroundColor Yellow + $winner = $null + } elseif ($winner.winner -in @('pr','pr-plus-reviewer') -and $winner.isPRFix -ne $true) { + Write-Host " ⚠️ winner.json: '$($winner.winner)' must have isPRFix=true — overriding" -ForegroundColor Yellow + $winner.isPRFix = $true + } elseif ($winner.winner -like 'try-fix-*' -and $winner.isPRFix -ne $false) { + Write-Host " ⚠️ winner.json: '$($winner.winner)' must have isPRFix=false — overriding" -ForegroundColor Yellow + $winner.isPRFix = $false + } + if ($winner -and $winner.isPRFix -eq $false -and [string]::IsNullOrWhiteSpace($winner.candidateDiff)) { + Write-Host " ⚠️ winner.json: non-PR winner has empty candidateDiff — falling back to PR-fix path" -ForegroundColor Yellow + $winner = $null + } + if ($winner) { + Write-Host " 🏆 Winning candidate: $($winner.winner) (isPRFix=$($winner.isPRFix))" -ForegroundColor Cyan + } + } catch { + Write-Host " ⚠️ Failed to parse winner.json (non-fatal): $_ — falling back to PR-fix path" -ForegroundColor Yellow + $winner = $null + } +} else { + Write-Host " ℹ️ No winner.json — defaulting to PR-fix posting path" -ForegroundColor Gray +} + +$isPRWinner = (-not $winner) -or ($winner.isPRFix -eq $true) + +if ($isPRWinner) { + # Post inline review comments (file:line findings from expert-reviewer agent) + $inlineScript = Join-Path $summaryScriptsDir "post-inline-review.ps1" + $findingsFile = Join-Path $RepoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json" + if ((Test-Path $inlineScript) -and (Test-Path $findingsFile)) { + try { + Write-Host " 📝 Posting inline review comments..." -ForegroundColor Cyan + if ($DryRun) { + & $inlineScript -PRNumber $PRNumber -FindingsFile $findingsFile -DryRun + } else { + & $inlineScript -PRNumber $PRNumber -FindingsFile $findingsFile + } + Write-Host " ✅ Inline review comments posted" -ForegroundColor Green + } catch { + Write-Host " ⚠️ Inline review posting failed (non-fatal): $_" -ForegroundColor Yellow + } + } else { + if (-not (Test-Path $findingsFile)) { + Write-Host " ℹ️ No inline findings file — agent may not have produced findings" -ForegroundColor Gray + } + } +} else { + # Non-PR candidate won — submit a REQUEST_CHANGES review with the candidate diff in the body + Write-Host " 📝 Non-PR candidate won — submitting REQUEST_CHANGES review with candidate diff..." -ForegroundColor Cyan + + $maxDiffBytes = 55KB + $diff = [string]$winner.candidateDiff + $truncated = $false + # Truncate by binary-searching the largest character count whose UTF-8 + # encoding fits within the byte budget (reserving room for the marker line). + # O(log n) and much cheaper than the previous O(n²) trim-512-and-recount loop. + $marker = "`n... [truncated]" + $markerBytes = [System.Text.Encoding]::UTF8.GetByteCount($marker) + $budget = $maxDiffBytes - $markerBytes + if ([System.Text.Encoding]::UTF8.GetByteCount($diff) -gt $maxDiffBytes) { + $lo = 0 + $hi = $diff.Length + while ($lo -lt $hi) { + $mid = [int](($lo + $hi + 1) / 2) + $bytes = [System.Text.Encoding]::UTF8.GetByteCount($diff.Substring(0, $mid)) + if ($bytes -le $budget) { $lo = $mid } else { $hi = $mid - 1 } + } + $diff = $diff.Substring(0, $lo) + $marker + $truncated = $true + } + + # Compute an outer code fence longer than any backtick run inside the diff + # (minimum 4) so the diff content cannot accidentally close the fence and + # leak into the surrounding markdown. Preserves the diff text exactly. + $maxBacktickRun = 0 + foreach ($m in [regex]::Matches($diff, '`+')) { + if ($m.Length -gt $maxBacktickRun) { $maxBacktickRun = $m.Length } + } + $fenceLen = [Math]::Max(4, $maxBacktickRun + 1) + $fence = '`' * $fenceLen + + $rationale = if ($winner.summary) { [string]$winner.summary } else { "Automated review identified a stronger candidate fix." } + $reviewBody = @" +🤖 **Automated review — alternative fix proposed** + +The expert-reviewer evaluation compared the PR fix against $($winner.winner -replace 'try-fix-','#') automatically generated candidates and selected ``$($winner.winner)`` as the strongest fix. + +**Why:** $rationale + +Please consider applying the candidate diff below (or use it as guidance). Once you push an update, this workflow will re-trigger and re-evaluate. + +
Candidate diff (``$($winner.winner)``) + +${fence}diff +$diff +$fence + +
+$( if ($truncated) { "`n_The diff was truncated to fit GitHub's review body limit._" } ) +"@ + + if ($DryRun) { + Write-Host " [DryRun] Would POST review state=REQUEST_CHANGES with body length $($reviewBody.Length)" -ForegroundColor Yellow + } else { + try { + $bodyJson = @{ body = $reviewBody; event = 'REQUEST_CHANGES' } | ConvertTo-Json -Compress -Depth 5 + $tmp = New-TemporaryFile + Set-Content -LiteralPath $tmp -Value $bodyJson -Encoding utf8 -NoNewline + $resp = & gh api -X POST "repos/dotnet/maui/pulls/$PRNumber/reviews" --input $tmp 2>&1 + Remove-Item -LiteralPath $tmp -Force -ErrorAction SilentlyContinue + if ($LASTEXITCODE -eq 0) { + Write-Host " ✅ REQUEST_CHANGES review submitted" -ForegroundColor Green + } else { + Write-Host " ⚠️ Failed to submit REQUEST_CHANGES review (non-fatal): $resp" -ForegroundColor Yellow + } + } catch { + Write-Host " ⚠️ REQUEST_CHANGES submission threw (non-fatal): $_" -ForegroundColor Yellow + } + } + Write-Host " ⏭️ Skipping inline findings (winner is not the PR fix)" -ForegroundColor Gray +} + # ═════════════════════════════════════════════════════════════════════════════ # STEP 4: Apply Labels # ═════════════════════════════════════════════════════════════════════════════ diff --git a/.github/scripts/post-inline-review.ps1 b/.github/scripts/post-inline-review.ps1 new file mode 100644 index 000000000000..e4fe45f509df --- /dev/null +++ b/.github/scripts/post-inline-review.ps1 @@ -0,0 +1,264 @@ +#!/usr/bin/env pwsh +<# +.SYNOPSIS + Posts inline review comments on a GitHub Pull Request from a JSON findings file. + +.DESCRIPTION + Reads inline-findings.json (produced by the maui-expert-reviewer agent) and posts + them as a GitHub PR review with inline file:line comments. + + Also posts review-summary.md as the review body. + + Uses the GitHub Pulls Review API to create a single review with all inline comments + attached at their exact file:line locations. + +.PARAMETER PRNumber + The pull request number (required) + +.PARAMETER FindingsFile + Path to inline-findings.json. Default: CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/inline-findings.json + +.PARAMETER SummaryFile + Path to review-summary.md. Default: CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/review-summary.md + +.PARAMETER DryRun + Print the review payload instead of posting + +.EXAMPLE + ./post-inline-review.ps1 -PRNumber 12345 + +.EXAMPLE + ./post-inline-review.ps1 -PRNumber 12345 -DryRun + +.EXAMPLE + ./post-inline-review.ps1 -PRNumber 12345 -FindingsFile /tmp/findings.json +#> + +param( + [Parameter(Mandatory = $true)] + [int]$PRNumber, + + [Parameter(Mandatory = $false)] + [string]$FindingsFile, + + [Parameter(Mandatory = $false)] + [string]$SummaryFile, + + [Parameter(Mandatory = $false)] + [switch]$DryRun +) + +$ErrorActionPreference = "Stop" + +# ============================================================================ +# RESOLVE FILE PATHS +# ============================================================================ + +$PRAgentDir = "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent" +if (-not (Test-Path $PRAgentDir)) { + $repoRoot = git rev-parse --show-toplevel 2>$null + if ($repoRoot) { + $PRAgentDir = Join-Path $repoRoot "CustomAgentLogsTmp/PRState/$PRNumber/PRAgent" + } +} + +if (-not $FindingsFile) { + $FindingsFile = Join-Path $PRAgentDir "inline-findings.json" +} +if (-not $SummaryFile) { + $SummaryFile = Join-Path $PRAgentDir "review-summary.md" +} + +if (-not (Test-Path $FindingsFile)) { + Write-Host "No findings file found at: $FindingsFile" -ForegroundColor Yellow + Write-Host "Nothing to post." -ForegroundColor Yellow + exit 0 +} + +# ============================================================================ +# LOAD FINDINGS +# ============================================================================ + +Write-Host "Loading findings from: $FindingsFile" -ForegroundColor Cyan +$findings = Get-Content -Path $FindingsFile -Raw -Encoding UTF8 | ConvertFrom-Json + +if (-not $findings -or $findings.Count -eq 0) { + Write-Host "No findings to post." -ForegroundColor Green + exit 0 +} + +Write-Host " Found $($findings.Count) inline findings" -ForegroundColor Gray + +# Load summary if available +$summaryBody = "" +if (Test-Path $SummaryFile) { + $summaryBody = Get-Content -Path $SummaryFile -Raw -Encoding UTF8 + Write-Host " Loaded summary ($($summaryBody.Length) chars)" -ForegroundColor Gray +} else { + $summaryBody = "## Expert Review — $($findings.Count) findings`n`nSee inline comments for details." +} + +# ============================================================================ +# GET PR HEAD COMMIT (required by GitHub API) +# ============================================================================ + +Write-Host "Fetching PR #$PRNumber head commit..." -ForegroundColor Cyan +$prJson = gh api "repos/dotnet/maui/pulls/$PRNumber" --jq '{sha: .head.sha}' 2>&1 +if ($LASTEXITCODE -ne 0) { + throw "Failed to fetch PR #${PRNumber}: $prJson" +} +$prData = $prJson | ConvertFrom-Json +$commitSha = $prData.sha +Write-Host " HEAD: $commitSha" -ForegroundColor Gray + +# ============================================================================ +# BUILD REVIEW PAYLOAD +# ============================================================================ + +$comments = @() +foreach ($f in $findings) { + # Defense-in-depth: reject suspicious paths so a malformed/hostile finding + # cannot poison the whole review post (especially in the fallback branch + # below where the GitHub diff fetch failed and we can't cross-validate). + $p = [string]$f.path + if ([string]::IsNullOrWhiteSpace($p) -or + $p.Contains('..') -or + $p.StartsWith('/') -or + $p.StartsWith('\') -or + $p.Contains('\') -or + $p -match '[\x00-\x1F]' -or + $p -match '^[A-Za-z]:') { + Write-Host " ⚠️ Skipping finding with suspicious path: '$p'" -ForegroundColor Yellow + continue + } + + $comment = @{ + path = $p + line = [int]$f.line + body = $f.body + } + # GitHub API requires 'side' for pull request review comments + $comment['side'] = 'RIGHT' + $comments += $comment +} + +# ============================================================================ +# FILTER COMMENTS TO LINES PRESENT IN THE PR DIFF +# GitHub returns 422 "Line could not be resolved" if ANY comment targets a +# line outside the diff. Pre-validate to avoid losing the entire review. +# ============================================================================ + +Write-Host "Fetching PR diff for line validation..." -ForegroundColor Cyan +$filesJson = gh api --paginate "repos/dotnet/maui/pulls/$PRNumber/files" 2>&1 +if ($LASTEXITCODE -ne 0) { + Write-Host " ⚠️ Could not fetch PR files for validation: $filesJson" -ForegroundColor Yellow + Write-Host " Posting all findings without pre-validation." -ForegroundColor Yellow +} else { + $files = $filesJson | ConvertFrom-Json + # Build map: path -> set of new-file line numbers commentable on RIGHT side + $diffLines = @{} + foreach ($file in $files) { + $path = $file.filename + $patch = $file.patch + if (-not $patch) { continue } + $lineSet = New-Object System.Collections.Generic.HashSet[int] + $newLine = 0 + foreach ($pl in ($patch -split "`n")) { + if ($pl -match '^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@') { + $newLine = [int]$Matches[1] + continue + } + if ($pl.StartsWith('+') -and -not $pl.StartsWith('+++')) { + [void]$lineSet.Add($newLine) + $newLine++ + } elseif ($pl.StartsWith('-') -and -not $pl.StartsWith('---')) { + # deletion — does not advance new-file line + } elseif ($pl.StartsWith(' ')) { + # context — also commentable (RIGHT side) + [void]$lineSet.Add($newLine) + $newLine++ + } + } + $diffLines[$path] = $lineSet + } + + $kept = @() + $dropped = @() + foreach ($c in $comments) { + if ($diffLines.ContainsKey($c.path) -and $diffLines[$c.path].Contains([int]$c.line)) { + $kept += $c + } else { + $dropped += $c + } + } + if ($dropped.Count -gt 0) { + Write-Host " ⚠️ Dropping $($dropped.Count) finding(s) whose lines aren't in the PR diff:" -ForegroundColor Yellow + foreach ($d in $dropped) { + Write-Host " $($d.path):$($d.line)" -ForegroundColor Gray + } + } + Write-Host " ✅ $($kept.Count) of $($comments.Count) findings target lines in the diff" -ForegroundColor Gray + $comments = $kept +} + +if ($comments.Count -eq 0) { + Write-Host "No inline-commentable findings remain after diff validation. Skipping review post." -ForegroundColor Yellow + exit 0 +} + +$reviewPayload = @{ + commit_id = $commitSha + body = $summaryBody + event = "COMMENT" # Never APPROVE or REQUEST_CHANGES — that's a human decision + comments = $comments +} + +$payloadJson = $reviewPayload | ConvertTo-Json -Depth 10 + +# ============================================================================ +# DRY RUN +# ============================================================================ + +if ($DryRun) { + Write-Host "" + Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow + Write-Host " DRY RUN — Review preview (not posted)" -ForegroundColor Yellow + Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow + Write-Host "" + Write-Host "Summary:" -ForegroundColor Cyan + Write-Host $summaryBody + Write-Host "" + Write-Host "Inline comments ($($comments.Count)):" -ForegroundColor Cyan + foreach ($c in $comments) { + Write-Host " $($c.path):$($c.line)" -ForegroundColor White -NoNewline + Write-Host " — $($c.body.Substring(0, [Math]::Min($c.body.Length, 120)))..." -ForegroundColor Gray + } + Write-Host "" + Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow + Write-Host " Payload size: $($payloadJson.Length) chars" -ForegroundColor Gray + Write-Host "═══════════════════════════════════════════════════════" -ForegroundColor Yellow + return +} + +# ============================================================================ +# POST REVIEW +# ============================================================================ + +Write-Host "Posting review with $($comments.Count) inline comments..." -ForegroundColor Cyan + +$tempFile = [System.IO.Path]::GetTempFileName() +try { + $payloadJson | Set-Content -Path $tempFile -Encoding UTF8 + + $result = gh api --method POST "repos/dotnet/maui/pulls/$PRNumber/reviews" --input $tempFile 2>&1 + if ($LASTEXITCODE -ne 0) { + throw "Failed to post review: $result" + } + + $reviewData = $result | ConvertFrom-Json + Write-Host "Review posted (ID: $($reviewData.id))" -ForegroundColor Green + Write-Host " $($comments.Count) inline comments at file:line" -ForegroundColor Gray + Write-Host " URL: $($reviewData.html_url)" -ForegroundColor Gray +} finally { + Remove-Item -Path $tempFile -Force -ErrorAction SilentlyContinue +} diff --git a/.github/scripts/shared/Detect-TestsInDiff.ps1 b/.github/scripts/shared/Detect-TestsInDiff.ps1 index 253b98e5a867..b7a141d4e034 100644 --- a/.github/scripts/shared/Detect-TestsInDiff.ps1 +++ b/.github/scripts/shared/Detect-TestsInDiff.ps1 @@ -185,6 +185,40 @@ $IgnoredFileNames = @( # Intermediate: collect test files grouped by type + test name $testGroups = @{} # Key: "Type:TestName" → Value: hashtable +# ─── Reliability fix #7: parse the actual class name from the .cs file ─── +# The previous logic derived the dotnet test filter from the *filename basename*, +# but maui's test repo uses a "category-prefix" file-naming convention where the +# filename includes a logical bucket dot the class name (e.g. +# `CarouselViewUITests.ProgrammaticPositionBounceBack.cs` containing the class +# `CarouselViewProgrammaticPositionBounceBack`). Filtering by the filename +# yielded `--filter FullyQualifiedName~CarouselViewUITests.ProgrammaticPositionBounceBack` +# which matched zero tests, and the gate marked the PR FAILED purely because of +# our auto-detection mistake. Reading the actual `public class` declaration +# from the file content is more reliable. Falls back to the filename basename +# when the file can't be read (e.g., file deleted, path unresolvable). +$RepoRootForRead = git rev-parse --show-toplevel 2>$null +function Get-ClassNameFromFile { + param([string]$RelativePath) + $candidates = @($RelativePath) + if ($RepoRootForRead) { + $candidates += (Join-Path $RepoRootForRead $RelativePath) + } + foreach ($p in $candidates) { + if (Test-Path $p) { + try { + $content = Get-Content $p -Raw -ErrorAction Stop + } catch { continue } + # Match the first non-static, non-abstract `public class XXX` or + # `public partial class XXX` declaration — only concrete classes (skip + # `abstract`/`static`) so a base test class declared above the concrete + # test class isn't picked up and turned into a non-matching test filter. + $m = [regex]::Match($content, '(?m)^\s*public(?:\s+(?:partial|sealed))*\s+class\s+(\w+)') + if ($m.Success) { return $m.Groups[1].Value } + } + } + return $null +} + foreach ($file in $ChangedFiles) { # Skip non-code files if ($file -notmatch "\.(cs|xaml)$") { continue } @@ -207,7 +241,14 @@ foreach ($file in $ChangedFiles) { # Only Shared.Tests files define actual test classes. # HostApp files are UI pages associated with tests but aren't tests themselves. if ($file -match "TestCases\.Shared\.Tests") { - if ($file -match "[/\\]([^/\\]+)\.cs$") { + # Prefer the class name parsed from the file content over + # the filename basename. maui's test repo often uses a + # category-prefix in the filename that does NOT match the + # actual class name (e.g. CarouselViewUITests.X.cs → class X). + $parsedClass = Get-ClassNameFromFile -RelativePath $file + if ($parsedClass) { + $testName = $parsedClass + } elseif ($file -match "[/\\]([^/\\]+)\.cs$") { $testName = $matches[1] } $filter = $testName @@ -223,7 +264,10 @@ foreach ($file in $ChangedFiles) { } "XamlUnitTest" { - if ($file -match "[/\\]([^/\\]+)\.(cs|xaml)$") { + $parsedClass = Get-ClassNameFromFile -RelativePath $file + if ($parsedClass) { + $testName = $parsedClass + } elseif ($file -match "[/\\]([^/\\]+)\.(cs|xaml)$") { $testName = $matches[1] $testName = $testName -replace '\.(rt|rtsg|rtxc|xaml)$', '' } @@ -232,7 +276,10 @@ foreach ($file in $ChangedFiles) { } "DeviceTest" { - if ($file -match "[/\\]([^/\\]+)\.cs$") { + $parsedClass = Get-ClassNameFromFile -RelativePath $file + if ($parsedClass) { + $testName = $parsedClass + } elseif ($file -match "[/\\]([^/\\]+)\.cs$") { $className = $matches[1] # Strip platform suffix: EditorTests.iOS → EditorTests $className = $className -replace '\.(iOS|Android|Windows|MacCatalyst)$', '' @@ -256,7 +303,10 @@ foreach ($file in $ChangedFiles) { } "UnitTest" { - if ($file -match "[/\\]([^/\\]+)\.cs$") { + $parsedClass = Get-ClassNameFromFile -RelativePath $file + if ($parsedClass) { + $testName = $parsedClass + } elseif ($file -match "[/\\]([^/\\]+)\.cs$") { $testName = $matches[1] } diff --git a/.github/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index 3f0b145ffa04..1a8d1bb0b37b 100644 --- a/.github/skills/code-review/SKILL.md +++ b/.github/skills/code-review/SKILL.md @@ -2,10 +2,11 @@ name: code-review description: >- Deep code review of PR changes for correctness, safety, and MAUI conventions. - Uses independence-first assessment (code before narrative) with 345 lines of - maintainer-sourced review rules. Triggers on: "review code for PR", "code review PR", - "analyze code changes", "check PR code quality". Do NOT use for: summarizing PRs, - describing what changed, general PR questions, running tests, or fixing code. + Uses independence-first assessment (code before narrative) and delegates to the + maui-expert-reviewer agent for per-dimension sub-agent evaluation. Triggers on: + "review code for PR", "code review PR", "analyze code changes", "check PR code quality". + Do NOT use for: summarizing PRs, describing what changed, general PR questions, + running tests, or fixing code. --- # Code Review Skill @@ -17,7 +18,7 @@ Standalone skill that evaluates PR code changes for correctness, safety, perform **Do NOT use for:** "what does PR #XXXXX do?", "summarize PR", "describe the changes", or any informational query — just answer those directly without invoking this skill. > **How this differs from other skills:** -> - **`pr-review`** — End-to-end PR workflow (3 phases: pre-flight with code review, try-fix, report; gate runs separately). Use when you want the full pipeline including test verification and fix attempts. Pre-Flight invokes this skill as a sub-agent for independence-first code analysis. +> - **`pr-review`** — End-to-end PR workflow (4 phases: pre-flight, gate, try-fix, report). Use when you want the full pipeline including test verification and fix attempts. > - **`pr-finalize`** — Verifies PR title/description match implementation + light code review. Use before merging. > - **`code-review`** (this skill) — Deep code-only review with MAUI domain rules. Use when you want a thorough code analysis without running tests or modifying the PR. @@ -71,9 +72,24 @@ Standalone skill that evaluates PR code changes for correctness, safety, perform git log --oneline -10 -- ``` -### Step 2: Load Review Rules +### Step 2: Delegate to Expert Reviewer -Read `.github/skills/code-review/references/review-rules.md`. These rules are distilled from real code reviews by senior MAUI maintainers across 142 high-discussion PRs. +Delegate to the `maui-expert-reviewer` agent (`.github/agents/maui-expert-reviewer.md`) which runs per-dimension sub-agent evaluation. The agent's sole output is `inline-findings.json` — file:line comments in GitHub Review API format. + +**After the agent finishes:** + +- **If `COMMENTS_VIA_FILE=true`** (CI): Done. The pipeline calls `post-inline-review.ps1` to post findings using `GH_COMMENT_TOKEN`. +- **If `COMMENTS_VIA_FILE` is unset** (local): Post inline findings directly: + ```bash + COMMIT_SHA=$(gh pr view $PR_NUMBER --json headRefOid --jq .headRefOid) + gh api repos/dotnet/maui/pulls/$PR_NUMBER/reviews \ + --method POST \ + --input <(jq -n \ + --arg sha "$COMMIT_SHA" \ + --arg body "Expert review — see inline comments." \ + --argjson comments "$(cat CustomAgentLogsTmp/PRState/$PR_NUMBER/PRAgent/inline-findings.json)" \ + '{commit_id: $sha, body: $body, event: "COMMENT", comments: [$comments[] | {path, line, body, side: "RIGHT"}]}') + ``` ### Step 3: Form Independent Assessment @@ -82,7 +98,7 @@ Based ONLY on the code (no PR description), answer: 1. **What does this change do?** Describe the behavioral change in your own words 2. **Why might it be needed?** Infer motivation from the code 3. **Is the approach sound?** Would a simpler alternative work? -4. **What problems do you see?** Run through the review rules AND the MAUI-Specific Review Checklist below +4. **What problems do you see?** Run through the agent's dimension CHECKs for matched dimensions ### Step 4: Read PR Narrative and Reconcile @@ -119,33 +135,6 @@ Then deliver your verdict: --- -## MAUI-Specific Review Rules - -Apply the rules in `references/review-rules.md` to each changed file. The rules are distilled from real code reviews across 142 high-discussion PRs and cover 20 categories: - -**Platform & Lifecycle:** Handler lifecycle, platform-specific code, Android, iOS/macCatalyst, Windows -**Architecture:** Memory management, threading, safe area, layout, navigation, CollectionView -**Code Quality:** Error handling, null safety, performance, XAML & bindings, API design -**Ecosystem:** Testing, build & MSBuild, image handling, gestures, accessibility - -The rules file also includes a **"What NOT to Flag"** section — respect it to avoid noise. - ---- - -## Multi-Model Review (Optional) - -When the environment supports multiple models, run the review in parallel for diverse perspectives. Different model families catch different classes of issues. - -**How:** -1. Select 2-3 models from different families (e.g., Claude + GPT + Gemini) -2. Launch sub-agents in parallel, each running the full 6-step workflow above -3. Synthesize: deduplicate findings, elevate issues flagged by multiple models -4. Present unified review noting which findings had multi-model agreement - -**Timeout:** If a sub-agent hasn't completed after 5 minutes, proceed with available results. - ---- - ## Review Output Format **Constraints (from Android team's approach):** @@ -194,23 +183,30 @@ When the environment supports multiple models, run the review in parallel for di 3. **Never approve what you can't verify.** If the fix touches platform code you can't fully reason about, say so explicitly and use `NEEDS_DISCUSSION`. 4. **LGTM means no ❌ Errors.** You can LGTM with 💡 Suggestions. You can LGTM with ⚠️ Warnings only if you've explained why they're acceptable. 5. **🚨 NEVER use `--approve` or `--request-changes` on GitHub.** Only post comments. Approval is a human decision. -6. **Output to terminal only by default.** Do not post review comments to GitHub (`gh pr review --comment`) unless explicitly asked by the user or orchestrated by another agent. This matches `pr-finalize` policy. +6. **Write findings to disk, do not post directly.** The agent does not have the GitHub comment token. Write findings to `CustomAgentLogsTmp/PRState/{PR}/PRAgent/` — the CI pipeline or posting scripts handle GitHub interaction. --- ## Posting the Review -After completing your review, suggest using the `Post-CodeReview.ps1` script to format and post the comment. **Do NOT post automatically** - always let the user decide when to post. +The agent writes findings to disk. Posting is done separately by `Review-PR.ps1`: +**Inline review comments** (preferred — findings at exact file:line): ```bash -# Save your review to a file, then suggest: -pwsh .github/scripts/Post-CodeReview.ps1 -PRNumber -ReviewFile /tmp/review.md -DryRun +# Preview first: +pwsh .github/scripts/post-inline-review.ps1 -PRNumber -DryRun + +# Post when ready: +pwsh .github/scripts/post-inline-review.ps1 -PRNumber +``` -# User can then post when ready: -pwsh .github/scripts/Post-CodeReview.ps1 -PRNumber -ReviewFile /tmp/review.md +**Wall-of-text summary** (phase content assembled into a single PR comment): +```bash +# Called by Review-PR.ps1 automatically: +pwsh .github/scripts/post-ai-summary-comment.ps1 ``` -The script wraps the review in a collapsible `
` section, adds PR metadata (commit SHA, title), and auto-detects the verdict for a colored status dot (🟢 Approved, 🟡 Changes Suggested, 🟠 Discussion Needed). +In CI (`eng/pipelines/ci-copilot.yml`), `Review-PR.ps1` calls both `post-inline-review.ps1` (for inline findings) and `post-ai-summary-comment.ps1` (for the wall-of-text from `{phase}/content.md` files), using `GH_COMMENT_TOKEN`. --- diff --git a/.github/skills/code-review/references/review-rules.md b/.github/skills/code-review/references/review-rules.md deleted file mode 100644 index 38831b6d740d..000000000000 --- a/.github/skills/code-review/references/review-rules.md +++ /dev/null @@ -1,345 +0,0 @@ -# .NET MAUI Review Rules - -Distilled from code reviews by senior maintainers of dotnet/maui. -Covers 142 high-discussion PRs with 2,883 review comments. - ---- - -## 1. Handler Lifecycle & Patterns - -Handlers bridge the cross-platform virtual view to the native platform view. -Lifecycle mistakes cause leaks, crashes on reconnect, and orphaned event subscriptions. - -| Check | What to look for | -|-------|-----------------| -| **ConnectHandler / DisconnectHandler symmetry** | Every listener, event handler, or callback registered in `ConnectHandler` must be unregistered in `DisconnectHandler`. Asymmetry is the #1 source of handler leaks. (PR #31022, PR #32278) | -| **Don't null handler references eagerly in Disconnect** | The view might be removed and re-added to the window (e.g., Shell tab switching). Setting `_handler = null` in `DisconnectHandler` breaks reconnection. Prefer clearing subscriptions while keeping the weak reference alive. (PR #7886) | -| **Assign per-view state in AttachedToWindow / DetachedFromWindow** | For Android, register layout listeners or inset listeners in `AttachedToWindow` and unregister in `DetachedFromWindow` rather than in the constructor. This avoids leaks when views are recycled. (PR #31022, PR #32278) | -| **Mapper methods must be idempotent** | Mapper callbacks can be called at any time, not just initial setup. They must fully initialize state from scratch — don't assume defaults or rely on prior calls. (PR #15512) | -| **Use `ModifyMapping` for Controls-layer overrides** | When adding mapper properties at the Controls level that override Core behavior, use `Mapper.ModifyMapping` so user-registered mappings aren't silently replaced. (PR #15512) | -| **Commands vs Mapper properties** | Commands (`CommandMapper`) are for requests from cross-platform to platform code (`ScrollTo`, `Remove`). Mapper properties associate with `BindableProperty` values. Don't mix the two patterns. (PR #15512) | -| **Base class calls ordering** | `ConnectHandler` should call `base.ConnectHandler()` before custom setup. `DisconnectHandler` should clean up before calling `base.DisconnectHandler()`. Reversed ordering can access disposed resources. (PR #32278) | -| **Centralize listener instances via static registry** | For listeners that are per-CoordinatorLayout or per-DrawerLayout (e.g., inset listeners), prefer a static registry method over instantiating a new listener object per call. Reduces allocations and centralizes lifecycle. (PR #32278) | - ---- - -## 2. Memory Management & Leak Prevention - -iOS and Android have fundamentally different GC behaviors. iOS's reference-counting -GC is especially sensitive to cycles. Android's Java/C# bridge creates hidden roots. - -| Check | What to look for | -|-------|-----------------| -| **Store handler references as `WeakReference`** | Platform views that communicate back to their handler must store `WeakReference` — not a strong reference. A strong handler ↔ platform view cycle prevents collection, especially on iOS. (PR #7886, PR #20124) | -| **Prefer delegates/Funcs over handler references** | Layout code uses `Func<>` callbacks to communicate without coupling to handler instances. Adopt this pattern: the platform view stores a `Func<>` rather than an `IViewHandler` reference. (PR #7886) | -| **Prefer static callbacks on iOS** | iOS tends to do better cleaning things up when the callback target is a static method. Move gesture recognizer callbacks and event handlers into static methods where feasible, passing state through the sender/tag. (PR #7886) | -| **Unsubscribe and dispose Android listeners** | Android listeners that extend `Java.Lang.Object` should be unsubscribed (`view.SetOnXxxListener(null)`) in `DisconnectHandler` to remove Java's reference. Also call `_listener?.Dispose(); _listener = null;` to release the Java peer. The unsubscribe is often more important than the Dispose — if the handler is GC'd, the listener will be collected too, but Java's reference to the listener prevents that from happening. (PR #31022) | -| **Closures capturing UIKit views are leaks** | Lambdas that close over `UIView`, `UIScrollView`, or any `NSObject` subclass create hidden strong references that the iOS GC cannot break. Extract the view access into a local, use a weak reference capture, or mark the lambda `static` to force no captured variables at compile time. (PR #13499) | -| **NSString and other IDisposable iOS types** | Static `NSString` constants (e.g., notification names) should be declared once as `static readonly` fields, not allocated on every use. Repeated allocation of undisposed `NSString` increases memory pressure. (PR #13499) | -| **CollectionView attached property memory impact** | Data stored on `BindableObject` via attached properties persists for the CollectionView's lifetime. For per-item or per-layout data, store it on the handler instance instead. CollectionView memory is critical. (PR #29496) | -| **Add memory leak device tests for new subscriptions** | When adding new event subscriptions or handler references, consider adding a leak-detection device test that verifies the handler/view can be collected after disconnect. (PR #20124, PR #31022) | - ---- - -## 3. Safe Area (iOS/macCatalyst) - -Safe area is the most frequently regressed subsystem. macOS CI runs on macOS 14/15 -(~28px title bar inset), but local dev may use macOS 26+ (~0px). Fixes must pass CI. - -| Check | What to look for | -|-------|-----------------| -| **Opt-in model: only `ISafeAreaView` / `ISafeAreaView2` views** | Only views implementing `ISafeAreaView` should receive safe area adjustments. Non-safe-area views must return empty padding, not device insets. (PR #30337) | -| **No double-padding across ancestor chain** | Before applying safe area adjustments, walk ancestors checking if any already handle the same edges. Edge-aware: parent handling `Top` does not block child handling `Bottom`. (PR #30337, PR #15512) | -| **Raw vs adjusted inset comparison** | `_safeArea` is filtered by `GetSafeAreaForEdge` (zeros edges per `SafeAreaRegions`); raw `UIView.SafeAreaInsets` includes all edges. Never compare them — compare raw-to-raw or adjusted-to-adjusted. (PR #30337) | -| **Never gate view callbacks on window-level insets** | Comparing `Window.SafeAreaInsets` to filter per-view `SafeAreaInsetsDidChange` callbacks blocks legitimate updates. On macCatalyst with custom TitleBar, the view's insets change without the window's changing, causing a 28px shift on CI. (PR #30337) | -| **Use constants for magic strings** | Property names like `"SafeAreaInsets"` used in mapper lookups must be constants (e.g., `PlatformConfiguration.iOSSpecific.Page.SafeAreaInsetsProperty.PropertyName`). Bare strings will break silently if renamed. (PR #15512) | -| **New safe area types belong in `src/Core`** | Types like `SafeAreaRegions` must be in `Core.csproj` so that `ISafeAreaView2` can reference them. Don't add core interface dependencies to `Controls.csproj`. (PR #30337) | -| **Don't use ISafeAreaView2 if ISafeAreaView can be extended** | Before creating a versioned interface like `ISafeAreaView2`, check if the existing interface can be extended with default interface methods or deprecated members. Prefer extending over versioning. (PR #30337 — StephaneDelcroix) | - ---- - -## 4. Layout & Measure - -Layout is the hot path. Mistakes cause infinite measure loops, incorrect sizing, -and subtle clipping issues that only manifest on specific device/OS combinations. - -| Check | What to look for | -|-------|-----------------| -| **Propagate changes through virtual view, not INCC** | Collection changes (e.g., map elements, picker items) should propagate via `Handler.UpdateValue(PropertyName)` at the Controls level, not via `INotifyCollectionChanged` from the platform view. INCC creates tight coupling. (PR #7886) | -| **Guard against infinite ContentSize oscillation** | On iOS, `MauiScrollView` can enter infinite loops when content with very large `ContentSize` triggers layout → resize → layout cycles. Add a guard (e.g., `EqualsAtPixelLevel` comparison) to break sub-pixel oscillation caused by animation noise. (PR #26629, PR #30337) | -| **Compare sizes at pixel resolution** | Safe area and content size comparisons must account for device-pixel rounding. Use `EqualsAtPixelLevel` (threshold ~0.0000001pt) to prevent oscillation during `TranslateToAsync` animations. (PR #30337) | -| **Don't apply padding in aspect ratio calculations** | When computing image aspect ratios on iOS, include padding for sizing but not for the aspect ratio computation itself. Padding should be added after the aspect ratio is applied. (PR #26629) | -| **Matrix test nesting for visibility propagation** | When testing `IsVisible` propagation through nested containers, test the full matrix: 2-level and 3-level nesting, set/unset sequences. A single level test misses propagation bugs. (PR #20154 — mattleibow) | - ---- - -## 5. Platform-Specific Code - -| Check | What to look for | -|-------|-----------------| -| **File extensions determine compilation targets** | `.android.cs` → Android only. `.ios.cs` → iOS AND MacCatalyst. `.maccatalyst.cs` → MacCatalyst only. `.windows.cs` → Windows only. Both `.ios.cs` and `.maccatalyst.cs` compile for MacCatalyst — check for duplicate definitions. (PR #24396) | -| **Use type aliases for namespace collisions** | `View`, `Color`, `Context` exist in both MAUI and Android/iOS/Windows namespaces. Use `using AView = Android.Views.View;`, `using NativeScrollView = Microsoft.UI.Xaml.Controls.ScrollViewer;`, etc. Prevents subtle misresolution bugs. (PR #3351 — mattleibow) | -| **Platform views must live in `Microsoft.Maui.Platform` namespace** | Custom native view wrappers (e.g., `MauiHybridWebView`, `MauiHybridWebViewClient`) must be in `Platform//` with `namespace Microsoft.Maui.Platform`. Don't put them in handler directories. (PR #22880 — mattleibow) | -| **Don't change handler generic type parameters** | Changing the generic type on a handler (e.g., `ViewHandler` → `ViewHandler`) is a binary breaking API change. Leave existing generics as-is. (PR #13499 — PureWeen) | -| **Align cross-platform behavior** | When fixing behavior on one platform, verify the behavior is consistent on others. A flyout state fix on iOS should match Windows behavior. Reviewers will ask: "What does Windows/Android do here?" (PR #26701 — jsuarezruiz) | -| **Reuse platform APIs via extension methods** | If a platform already has a method (e.g., `FileSystemPlatformOpenAppPackageFile`), use it. Don't duplicate file-opening logic in handler-specific code. (PR #22880 — mattleibow) | - ---- - -## 6. Android Platform - -| Check | What to look for | -|-------|-----------------| -| **Store `Context` in a local before repeated use** | Accessing Android's `Context` property calls into Java each time (JNI marshaling). Store it in a local: `var context = Context;` then use the local. Same applies to any property that crosses the Java/C# bridge. (PR #26789 — jonathanpeppers) | -| **Android resource files need correct build action** | Files used in Android tests must be declared with the correct build action (e.g., `AndroidResource`, `EmbeddedResource`). A file at `/red.png` with no build action causes `FileNotFoundException` on Android only. (PR #14109 — jonathanpeppers) | -| **Use `PlatformLogger` for Android native code** | In Java code under `src/Core/AndroidNative/`, use `PlatformLogger` for logging — not `android.util.Log` directly. This ensures consistent log formatting. (PR #29780 — jonathanpeppers) | -| **Use `FragmentActivity` carefully with Glide** | When resolving Glide request managers, check Glide's own source for the correct `Activity` lookup pattern. Don't add unnecessary `FragmentActivity` checks that diverge from Glide's implementation. (PR #29780 — jonathanpeppers) | -| **Tests in Java won't run without CI changes** | New Java test projects require AzDO/yaml/CI pipeline updates. If CI changes aren't included, the tests won't execute. Write device tests in C# instead. (PR #29780 — jonathanpeppers) | - ---- - -## 7. iOS/macCatalyst Platform - -| Check | What to look for | -|-------|-----------------| -| **iOS has trouble collecting objects with handler references** | When an iOS platform view holds a reference to its handler, the reference-counting GC often cannot break the cycle. Use delegates, `Func<>`, or `WeakReference` patterns. See `DatePickerHandler.ios.cs` (`DatePickerDelegate`) for the recommended proxy pattern. (PR #7886 — PureWeen) | -| **Subscribe/unsubscribe in `MovedToWindow`** | For iOS callbacks that need handler access, subscribe when the view moves to a window and unsubscribe when removed. This is more reliable than constructor/dispose for iOS lifecycle. (PR #7886 — PureWeen) | -| **Store notification `UserInfo` before repeated access** | Accessing `notification.UserInfo` multiple times with null-conditional (`?.`) is wasteful and obscures null checks. Store `var userInfo = notification.UserInfo;`, null-check once, then use the methods. (PR #13499 — mandel-macaque) | -| **Check `Handle == IntPtr.Zero` for disposed native objects** | A `UICollectionView` or other native object may be disposed but not null. Check `collectionView?.Handle == IntPtr.Zero` as an additional guard against accessing disposed objects. (PR #29397 — jsuarezruiz) | -| **Wrap CollectionView layout callbacks in try-catch** | Layout factory callbacks can fire after disposal. Wrap in `catch (Exception ex) when (ex is ObjectDisposedException or InvalidOperationException)` to prevent crashes. (PR #29397 — jsuarezruiz) | -| **`UIImage.FromImage` creates a copy** | Calling `UIImage.FromImage` allocates a new instance. If you only need to transform the existing image, modify in place when possible to avoid double memory usage. (PR #26016 — jsuarezruiz) | -| **Use `IUITextInput` interface for cursor/text range** | When accessing `SelectedTextRange` or similar text input properties across `UITextField`/`UITextView`, use the `IUITextInput` interface to avoid duplicating code for each concrete type. (PR #13499 — mandel-macaque) | -| **macCatalyst defaults `UseSafeArea` to `true`** | Unlike iOS where `UseSafeArea` defaults to `false`, macCatalyst defaults to `true`. Code that assumes `false` across Apple platforms will misbehave on macCatalyst. (PR #30337) | - ---- - -## 8. Windows Platform - -| Check | What to look for | -|-------|-----------------| -| **Appium on WinUI can't retrieve non-accessible elements** | `BoxView` and other elements without text aren't visible to the WinUI accessibility tree. Use `Label` or elements with text content for Appium-based UI test assertions on Windows. (PR #19371 — PureWeen) | -| **Check null/empty collections before `.FirstOrDefault()`** | On Windows, calling `.FirstOrDefault().ToPlatform()` on an empty (but non-null) `Accelerators` collection will throw. Add an empty-collection guard. (PR #14740 — mattleibow) | -| **WebView2 assembly identity differs between WinUI and WPF** | The WebView2 types have different assembly identities across WinUI, WPF, and WinForms. You cannot directly share WebView2 helper code between these targets. (PR #654 — Eilon) | -| **Prefix new MSBuild properties with `Maui`** | Any new MSBuild property (e.g., `EnableXamlLoading`) must be prefixed with `Maui` (e.g., `MauiEnableXamlLoading`) to avoid collisions with WindowsAppSdk or other framework properties. (PR #19310 — jonathanpeppers) | -| **Apply theme per window, not to all windows** | In `OnWindowHandlerChanged`, don't call `ApplyThemeToAllWindows()` — it iterates all windows redundantly (N+N-1+...+1 total applications for N windows). Apply theme only to the specific window from the sender. (PR #31714 — jsuarezruiz) | -| **Track applied state to avoid redundant work** | Theme and style application should track whether it's already been applied per window (e.g., using a `Dictionary`). Re-applying on every handler change without checking if the theme actually changed is a performance hit. (PR #31714 — jsuarezruiz) | - ---- - -## 9. Navigation & Shell - -| Check | What to look for | -|-------|-----------------| -| **Shell re-adds views on tab switch** | Shell removes and re-adds platform views when users switch tabs or flyouts. Code that nullifies state in `DisconnectHandler` or `RemovedFromSuperview` will break on re-navigation. (PR #7886 — PureWeen) | -| **Test flyout state after window resize AND rotation** | Flyout visibility bugs often differ between maximize/restore and rotate-to-landscape/rotate-back scenarios. Create separate tests for each rather than combining into a single test method. (PR #26701 — PureWeen) | -| **Split unrelated behaviors into separate tests** | When a test covers two different behaviors (e.g., flyout-after-maximize AND flyout-after-rotation), split into separate test methods. Combined tests obscure which scenario fails. (PR #26701 — PureWeen) | -| **Security: validate WebView URL mapping** | When loading local HTML content in `WebView`, the local file mapping hostname should be random or scoped to prevent cross-origin file access. An app calling `LoadHtml` with `baseUrl: null` could inadvertently expose local files. (PR #7672 — Eilon) | -| **More tab behavior is platform-specific** | iOS's "More" tab for overflow Shell items may not have a standard Apple-defined behavior. Verify against Apple HIG before assuming push navigation from the More tab is correct. (PR #26292 — mattleibow) | - ---- - -## 10. CollectionView - -Two handler implementations exist. Items/ is the only implementation for Android -and Windows. Items2/ is the current handler for iOS/MacCatalyst (Items/ iOS is deprecated). - -| Check | What to look for | -|-------|-----------------| -| **Verify correct handler directory for platform** | Android/Windows changes go in `Handlers/Items/`. iOS/MacCatalyst changes go in `Handlers/Items2/`. Items2/ has NO Android or Windows code. Modifying the wrong directory is a no-op on the target platform. (PR #29397, PR #31336) | -| **Don't copy already-marshaled ObjC arrays** | iOS binding APIs like `indexPathsForVisibleItems` already copy the ObjC array to managed code via `CFArray.ArrayFromHandle`. Creating a second copy (e.g., `.ToArray()`) is wasteful. (PR #31336 — jonathanpeppers) | -| **MeasureFirstItem cache must distinguish item types** | When `MeasureFirstItem` is used with grouped CollectionView, the GroupHeader size gets cached and applied to all items if the cache key doesn't distinguish between headers and data items. Causes clipping. (PR #27847 — PureWeen) | -| **CollectionView inside ScrollView causes infinite loops** | Placing a CollectionView inside a ScrollView (which users shouldn't do, but do) can cause infinite layout loops on iOS due to unbounded `ContentSize`. Add guards. (PR #26629 — rmarinho) | -| **Include gallery samples alongside tests** | When fixing EmptyView, DataTemplateSelector, or other complex CollectionView features, add samples to `Controls.Sample/Pages/Controls/CollectionViewGalleries/` for manual validation alongside automated tests. (PR #25418 — jsuarezruiz) | -| **Validate item class names are unique** | ViewModels inside test files (e.g., `CollectionViewViewModel`) must be namespaced uniquely (e.g., `Issue29130ViewModel`). Duplicate class names across issues cause build errors. (PR #29496 — jsuarezruiz) | - ---- - -## 11. Threading & Async - -| Check | What to look for | -|-------|-----------------| -| **Use `Interlocked.Increment` for counters accessed from UI thread** | Even counters "most likely" called on the UI thread should use `Interlocked` for safety. A debounce counter without thread safety can miss update requests. (PR #13499 — PureWeen) | -| **Don't reset debounce counters — roll them over** | Resetting a debounce count to 0 can cause race conditions where an update request is missed. Roll the counter over at a high threshold (e.g., 100) instead. (PR #13499 — PureWeen) | -| **Check if already on main thread before dispatching** | Before calling `MainThread.BeginInvokeOnMainThread` or `Dispatcher.Dispatch`, check if you're already on the main thread. Unnecessary dispatch adds latency and can reorder operations. (PR #26153 — PureWeen) | -| **`Task.Delay` in tests needs justification** | A `Task.Delay(100)` in a device test is a flaky test waiting to happen. Reviewers will ask: "How do we know 100ms is enough?" Prefer `WaitForMainThread()` or similar deterministic helpers. (PR #14619 — jonathanpeppers) | -| **Create helper methods for common async test patterns** | Repeated `await InvokeOnMainThreadAsync(() => { })` should be extracted into a `WaitForMainThread()` helper. Duplicate async boilerplate is error-prone. (PR #14619 — jonathanpeppers) | -| **Check `DispatcherQueue` for null before posting** | On Windows, `DispatcherQueue` can be null if the Window is disposed. Guard dispatch calls with a null check. (PR #31714 — jsuarezruiz) | - ---- - -## 12. XAML & Bindings - -| Check | What to look for | -|-------|-----------------| -| **Compiled bindings require explicit `x:DataType`** | Every `{Binding}` with an explicit `Source` must have an explicit `x:DataType` on the binding or a parent element. Missing `x:DataType` prevents source generation and causes runtime reflection fallback. (PR #32444 — simonrozsival) | -| **Set `x:DataType` on root element for page-level BindingContext** | When a page sets `BindingContext` in code-behind, the root `ContentPage` element needs `x:DataType="local:MyViewModel"` to enable compiled bindings for all child elements. (PR #32444 — simonrozsival) | -| **Don't subscribe on every BindingContext change** | In `TemplatedCell` and similar reusable views, subscribing to events on BindingContext change without checking for diff will accumulate multiple subscriptions. Only subscribe if the BindingContext actually changed. (PR #14619 — rmarinho) | -| **Use `SetValue(BP, ...)` when BindableProperty exists** | When a source generator heuristic determines a BindableProperty will be generated, the generated code must use `.SetValue(BP, ...)` instead of calling the property setter directly. (PR #32597 — simonrozsival) | -| **Remove dead code from source generation** | When source-gen skips properties via `SkipProperties`, the `CreateValuesVisitor` should also skip `new` instantiations for those properties. Leftover dead code confuses reviewers and wastes binary size. (PR #32474 — simonrozsival) | -| **Future-proof message types with records** | When adding a new cross-platform messaging API (e.g., `SendRawMessage`), pass a `record` type instead of a bare `string`. This allows adding fields later without breaking the API. (PR #22880 — mattleibow) | -| **Markup extension recognition should be semantic** | Don't just check suffix (`FooExtension`); query the compilation for types implementing `IMarkupExtension`. Also support prefixed extensions (`{local:MyExtension}`). (PR #33693 — simonrozsival) | -| **Auto-escape XML-unfriendly characters in expressions** | Users should not need to type `>` in XAML C# expressions. Use `` or automatic escaping for `<`, `>`, `&&`, `||` in expression contexts. (PR #33693 — simonrozsival) | - ---- - -## 13. Testing - -| Check | What to look for | -|-------|-----------------| -| **UI tests must run on all applicable platforms** | Do not restrict tests to a single platform unless there is a specific technical limitation. Tests should always cover iOS, Android, and Windows unless the fix is platform-specific. (PR #32064 — PureWeen) | -| **Snapshot baselines must be updated across all platforms** | Changing background color, font, or layout requires updating snapshot baselines for Android, iOS, MacCatalyst, and Windows. Stale baselines cause false failures. (PR #25129, PR #27399 — jsuarezruiz) | -| **Screenshot size must match capture method** | If the screenshot capture mechanism changes (e.g., different DPI or crop), all baselines need regeneration. A size mismatch (e.g., 1920×1080 vs 789×563) means the capture changed, not the rendering. (PR #25129 — jsuarezruiz) | -| **Use `VerifyScreenshot(retryTimeout:)` instead of `Task.Delay`** | `VerifyScreenshot` has built-in retry logic. Adding `Task.Delay` before it is redundant. Use `retryTimeout: TimeSpan.FromSeconds(2)` for animations. (PR #32064) | -| **Make test labels visible even if content is clipped** | When testing clipping or canvas rendering, position a label at the edge of the drawn content so screenshots prove content was actually drawn, not just that the clip removed everything. (PR #28353 — mattleibow) | -| **Memory consumption tests on Android** | Use Appium's `GetMemoryInfo()` helper to validate memory consumption in tests. Set a threshold and assert memory stays below it to detect regressions. (PR #26789 — jsuarezruiz) | -| **Tests that verify two behaviors need two methods** | A test called `ShouldFlyoutBeVisibleAfterMaximizingWindow` that also tests rotation is actually two tests. Split them so failures pinpoint the exact regression. (PR #26701 — PureWeen) | -| **Test types should match test project infrastructure** | Source generator tests belong in `SourceGen.UnitTests.csproj`, not `Xaml.UnitTests.csproj`. Tests that don't need `[Values] XamlInflator` shouldn't use it — running 3 identical iterations wastes CI time. (PR #32474 — simonrozsival) | - ---- - -## 14. Performance - -| Check | What to look for | -|-------|-----------------| -| **Cache JNI property access in locals** | Android properties like `Context`, `Resources`, `ContentDescription` cross the Java/C# bridge on every access. Store in a local variable when used more than once in a method. (PR #26789 — jonathanpeppers) | -| **Avoid closures that increase GC pressure** | Closures capturing UIKit objects create extra GC-tracked objects with references to captured elements. Even if technically correct, this increases memory pressure. Prefer explicit parameter passing or static methods. (PR #13499 — mandel-macaque) | -| **`ValueTuple.GetHashCode()` may allocate** | `ValueTuple.GetHashCode()` for large tuples can allocate. For hash-code–critical types like `SetterSpecificity`, implement `GetHashCode()` explicitly to avoid boxing and allocation. (PR #13818 — jonathanpeppers) | -| **Prefer `IReadOnlyList` on public APIs** | Public methods should return `IReadOnlyList` or `IReadOnlyCollection` instead of mutable `List`. This communicates intent and prevents callers from corrupting internal state. (PR #14740 — mattleibow) | -| **Avoid double-copy of managed arrays from ObjC** | iOS binding APIs already marshal ObjC arrays into C# collections. Calling `.ToArray()` or `.ToList()` on the result creates a redundant copy. (PR #31336 — jonathanpeppers) | -| **Use `StringBuilder` or `List` for code generation** | In source generators, building output strings with repeated `+=` is O(n²). Use `StringBuilder`, `List` with `string.Join`, or append patterns that avoid intermediate allocations. (PR #32420 — simonrozsival) | -| **Allocate early, check cheap conditions first** | In validation chains, test simple boolean flags and null checks before allocating strings or doing I/O. This is especially important in mapper callbacks which run frequently. (PR #28077 — jsuarezruiz) | -| **Don't remove caches without benchmarks** | If a cache (like `SetterSpecificityList`) was added with measured performance wins, removing or replacing it requires proving the alternative provides equivalent or better performance. Use `dotnet-trace` and `speedscope.app`. (PR #17756 — jonathanpeppers) | -| **Flatten nested LINQ into single-level iteration** | Nested `foreach` + `SelectMany` over chained mappers can be flattened: `foreach (var key in Chained.Reverse().SelectMany(c => c.GetKeys()))`. But always benchmark to verify improvement. (PR #28077 — jsuarezruiz) | - ---- - -## 15. Error Handling & Null Safety - -| Check | What to look for | -|-------|-----------------| -| **Check if stream is seekable before copying** | When processing image or data streams, check `stream.CanSeek` first. If seekable, use the original stream directly instead of copying to a `MemoryStream`. (PR #29769 — jsuarezruiz) | -| **Pattern match instead of cast + null check** | Prefer `if (localCursor is CGRect rect)` over `as` + null check. Pattern matching is safer with value types (where `as` doesn't compile) and more readable. (PR #13499 — tj-devel709) | -| **Consistent null initialization** | Don't initialize nullable reference variables to `null` in some places but not others. Be consistent — either always set `= null` or never set it (let the default apply). (PR #13499 — mandel-macaque) | -| **Use `?.` chains judiciously** | Multiple `?.` on the same expression (e.g., `notification.UserInfo?.ObjectForKey(...)?.ToString()`) is harder to debug. Store intermediates and null-check explicitly when the chain is more than 2 deep. (PR #13499 — mandel-macaque) | -| **Fallback when `MauiContext` is null** | If `Window.MauiContext` is null, fall back to `Application.Current.FindMauiContext()` rather than letting a `NullReferenceException` propagate. (PR #24808 — jsuarezruiz) | -| **Initialize WebView cookies in `CoreWebView2Initialized`** | Cookie preloading that runs before `CoreWebView2` is initialized hits null references. Wire initialization into the `CoreWebView2Initialized` event. (PR #24846 — PureWeen) | -| **Try-catch for fire-and-forget platform calls** | Platform calls that can fail but whose failures are non-critical should be wrapped in `try/catch` or use `FireAndForget`. Unhandled exceptions in these paths crash the app. (PR #22880 — PureWeen) | -| **Guard against empty collections in chained calls** | `collection?.FirstOrDefault().ToPlatform()` throws if the collection is empty (not null). Check `.Any()` or use null-conditional on the result of `FirstOrDefault()`. (PR #14740 — mattleibow) | - ---- - -## 16. API Design & Public API - -| Check | What to look for | -|-------|-----------------| -| **Adding to an interface is a breaking change** | Adding members to a public interface (e.g., `IMenuElement`) is a binary breaking change. Use default interface methods (verify platform support), create a new interface (`IMenuElement2`), or add an extension method. (PR #14740 — PureWeen) | -| **Never modify `PublicAPI.Shipped.txt`** | To remove a shipped API, copy the line to `PublicAPI.Unshipped.txt` prefixed with `*REMOVED*`. Never edit the Shipped file directly. (PR #14740 — mattleibow) | -| **Don't expose setters that do nothing** | If a property setter is not wired to any handler or has no observable effect, remove it. Dead setters confuse users and accumulate API surface that can't be removed later. (PR #14740 — PureWeen) | -| **Obsolete with proper message format** | Obsolete attributes need a period at the end of the message. iOS Cecil tests enforce this convention. (PR #13499 — mandel-macaque) | -| **Public `MauiAppBuilder` extension methods require careful review** | Once added to the builder pattern, extension methods cannot be removed. Establish a process for evaluating what belongs on the builder vs what library authors should register themselves. (PR #2137 — mattleibow) | -| **Avoid breaking Xamarin.Forms migration** | When replacing types (e.g., `Position` → `Location`), consider keeping a compatibility class that inherits from the new type. This eases porting from Xamarin.Forms. (PR #7886 — jsuarezruiz) | -| **Use named constants instead of magic numbers** | Bit-shift offsets, specificity values, and layout thresholds should be `const int` with descriptive names. `new SetterSpecificity(100, 0, 0, 0)` is opaque; `SetterSpecificity.Implicit` communicates intent. (PR #13818 — jonathanpeppers) | - ---- - -## 17. Image Handling - -| Check | What to look for | -|-------|-----------------| -| **Image source services must return native image representations** | `IImageSourceService.GetDrawableAsync` should return a result that represents the actual image data, not just a status. This enables usage without a view (e.g., notification icons). (PR #2360 — mattleibow) | -| **Verify Glide callback thread assumptions** | Glide callbacks like `onResourceReady` are assumed to be on the main thread because they call `view.setImageDrawable`. But this isn't documented — verify by checking Glide's source. (PR #14109 — jonathanpeppers) | -| **Clean up bitmaps on overlay add/remove cycles** | If an overlay is removed and re-added, resources loaded during the first add must be either cached or reloadable. Disposal without a reload path causes blank overlays. (PR #3351 — mattleibow) | -| **Inner vs outer corner radius for clipping** | When using a `RoundRectangle` for clipping (e.g., inside a `Border`), the corner radius for the inner clip is different from the outer border radius. Use the inner value. Clipping with the outer value leaves visible gaps. (PR #10964 — jsuarezruiz) | - ---- - -## 18. Gestures - -| Check | What to look for | -|-------|-----------------| -| **Use `Tap` over `Click` for mobile platforms** | In UI tests, `Tap` is more flexible and mobile-appropriate. `Click` may not be converted to `Tap` on all platforms, causing test failures on Android/iOS while passing on Windows. (PR #19371 — PureWeen) | -| **Ensure gesture tests verify their precondition** | A test that verifies "GetPosition returns correct coordinates" must confirm the element was actually tapped. If the element isn't tappable (e.g., iOS BoxView issue), the test passes trivially. Use a verifiable side effect. (PR #19371 — PureWeen) | -| **Span tap region calculation across multiple lines** | The `CGRect` region for each `Span` in a `FormattedString` is incorrectly calculated when text wraps to multiple lines (inherited from Xamarin.Forms). Verify tap targets with multi-line text. (PR #15544 — jsuarezruiz) | -| **EventArgs naming convention** | Custom event args should follow `{Event}EventArgs : EventArgs` naming. E.g., if the event is `Touch`, the args class should be `TouchEventArgs`, not `DiagnosticsTouchArgs`. (PR #3351 — mattleibow) | - ---- - -## 19. Build & MSBuild - -| Check | What to look for | -|-------|-----------------| -| **Use `dotnet-public` feed to avoid dependency confusion** | NuGet sources must use the `dotnet-public` feed. Adding arbitrary third-party feeds creates a dependency confusion attack surface. New package sources require approval. (PR #5020 — jonathanpeppers) | -| **Build tasks can't depend on optional packages** | `Controls.Build.Tasks.csproj` cannot reference optional NuGet packages like Maps. Build tasks must only depend on core assemblies that are always present. (PR #7886 — mattleibow) | -| **Feature flag properties belong in `src/Core`** | Feature flags (e.g., `IsXamlLoadingSupported`) should be in `Core.csproj` / `Microsoft.Maui.dll` since all assemblies depend on it. Don't scatter feature flags across Controls or platform-specific assemblies. (PR #19310 — jonathanpeppers) | -| **Follow naming conventions: `Support` suffix → `IsXSupported`** | MSBuild property: `XamlLoadingSupport`. C# property: `IsXamlLoadingSupported`. This matches the runtime feature switch pattern used across .NET. (PR #19310 — simonrozsival) | -| **Document feature switch breakage and alternatives** | For each new feature switch, document: (1) which APIs break when disabled, (2) code examples of what users should do instead. (PR #19310 — jonathanpeppers) | -| **Targets in NuGet vs workload have different import timing** | Moving MSBuild targets from workload to NuGet changes when they're imported relative to package restore. Test the sequence: install VS → new project → NuGet restore → build. (PR #11206 — jonathanpeppers) | -| **Never commit `cgmanifest.json` or `templatestrings.json`** | These are auto-generated during CI. AI-generated PRs frequently include changes to these files. Always reset them before committing. (PR #11206) | - ---- - -## 20. Accessibility - -| Check | What to look for | -|-------|-----------------| -| **Don't disable font scaling globally** | An implicit style that disables font scaling across the app makes it inaccessible to partially-sighted users. "Rather have an ugly app that a partially blind person can use instead of a beautiful one they can't." (PR #1774 — PureWeen) | -| **WinUI Appium requires accessible elements** | Elements without text content (e.g., `BoxView`) are invisible to Appium on WinUI. Use `Label` or set `AutomationProperties.Name` for elements that need to be located in Windows UI tests. (PR #19371 — PureWeen) | -| **Test accessibility properties propagate** | When a control exposes `AutomationProperties`, verify the native accessibility tree reflects them. A broken binding silently removes accessibility. (PR #25011 — jsuarezruiz) | - ---- - -## 21. Regression Prevention (Lessons from Reverts & Candidate Failures) - -Rules distilled from 30 reverted PRs and 50 candidate-branch failures. When a PR touches these areas, apply extra scrutiny. - -| Check | What to look for | -|-------|-----------------| -| **CollectionView changes need broad scenario coverage** | CV is the single highest-regression component (15 candidate failures). Any change to layout, scroll, spacing, cell alignment, Header/Footer, or `KeepLastItemInView` must be tested across all four: empty collection, single item, many items, and with grouping. A fix for one layout scenario routinely breaks another. (CollectionView candidate failures, multiple PRs) | -| **Style/theme changes have cascading effects** | `ApplyToDerivedTypes`, implicit styles, and `AppThemeBinding` interact in non-obvious ways. A fix to one style propagation path often breaks another — this pattern caused two separate reverts for PR #9648 and broke source gen in PR #32728. When touching style resolution or `AppThemeColor`, test: explicit style, implicit style, derived-type style, and dark/light theme switching. | -| **Test the fix scenario AND adjacent scenarios** | Most reverts happen because the fix works for the reported issue but breaks a neighboring case. ToolbarItem image fix (PR #28833, reverted twice) fixed one image mode while breaking others. Entry `SelectionLength` (PR #26213) fixed selection but broke focus. Require authors to enumerate what adjacent behaviors they checked. | -| **Never remove `InternalsVisibleTo` without auditing NuGet consumers** | IVT removal looks like cleanup but silently breaks community packages that depend on internal APIs. At least one revert went all the way back to a release branch because of this. Flag any `InternalsVisibleTo` deletion and ask: "Have you checked whether any published NuGet consumers reference this?" | -| **ToolbarItem/Image changes must be tested across all image source types** | Font images, file images, and tinted images each go through different code paths. PR #28833 (reverted twice) and PR #26048 show this pattern. When modifying `ImageSourceExtensions` or any ToolbarItem rendering code, verify all three modes: `FontImageSource`, `FileImageSource`, and `BitmapImageSource` with tinting. | -| **Entry/Editor focus and selection state is fragile** | `CursorPosition`, `SelectionLength`, keyboard show/hide, and focus order interact tightly on every platform. PR #26213 shows how a `SelectionLength` change broke Entry focus. Flag any handler change that touches these properties and ask whether focus behavior was verified after the change — especially when the keyboard is dismissed and re-shown. | -| **Measurement timing on iOS lags behind property changes** | `UIButton.TitleLabel.Bounds` and `UIView` frame values are not updated synchronously after a property change — the layout pass hasn't run yet. PR #25122 (tj-devel709) shows the correct pattern: measure the title manually instead of reading `.Bounds`. Flag any iOS handler code that reads frame/bounds immediately after setting a property, without deferring to the next layout pass. | -| **Template changes need all-template validation** | A template fix for MAUI app can break Blazor hybrid or multi-project templates. `MapStaticAssets` vs `UseStaticFiles` (candidate failure) shows how a single API swap breaks one template variant silently. Any `src/Templates/` change must be validated against all template IDs: `maui`, `maui-blazor`, `maui-blazor-web`, `mauilib`, `maui-multiproject`. | -| **Candidate-branch PRs must not mix concerns** | PureWeen's explicit rule (PR #33838): candidate fixes should not bundle unrelated flakiness fixes. A PR that mixes "fix the regression" with "also fix this flaky test" makes bisection impossible and obscures what actually fixed the candidate failure. Flag mixed-concern candidate PRs. | -| **Screenshot tests must prove content was drawn, not just that clipping worked** | mattleibow on PR #28353: "give this label a negative margin so that we can see that the image is drawn at all." A clipping test that passes when the view renders nothing is not a useful test. Flag screenshot tests for `GraphicsView`, `Image`, or any drawing surface that don't verify the content region itself — use a visible element inside the clip boundary as a sentinel. | -| **Arithmetic in index/position calculations needs explicit parentheses** | mattleibow on PR #23369: "This line of code is a bit ambiguous for a quick read." Silent operator-precedence bugs in scroll offset, index math, or spacing calculations are hard to spot and have caused gesture/tap regressions. Flag expressions like `a + b * c` or `offset - padding / 2` without explicit parentheses when they appear in position or size computations. | -| **Major dependency upgrades need broad platform validation** | WindowsAppSDK upgrade (PR #32174) was reverted because it broke too many things simultaneously. Flag PRs that bump `WindowsAppSDK`, `Microsoft.Maui.*` NuGet versions, or other platform SDK dependencies, and ask whether CI was green on all platforms (Android, iOS, MacCatalyst, Windows) before merge, not just the changed platform. | -| **ContentPresenter BindingContext propagation breaks explicit TemplateBindings** | Propagating `BindingContext` through `ContentPresenter` overwrites `{TemplateBinding}` values that were set explicitly. This was a reverted PR. Flag any handler or renderer change that sets or propagates `BindingContext` on a `ContentPresenter` or control template root without verifying that `TemplateBinding` expressions still resolve correctly. | - ---- - -## 22. Most Frequently Regressed Components - -Components ranked by regression frequency from 366 analyzed PRs (reverts + candidate fixes + regression fixes): - -| Component | Regressions | Key Risk Areas | -|-----------|-------------|----------------| -| CollectionView | 15 | Layout, scroll position, spacing, cell alignment, Header/Footer | -| Image/Graphics | 15 | Aspect ratio, CornerRadius, Background, DrawString | -| Theme/Style | 8 | AppThemeBinding, implicit styles, ApplyToDerivedTypes | -| CarouselView | 7 | ScrollTo, CurrentItem, ItemSpacing, loop mode | -| Gesture/Tap | 7 | TapGestureRecognizer, SwipeView, outside-tap dismiss | -| Button/Entry | 7 | Dynamic resize, focus/selection, AppThemeBinding colors | -| Toolbar | 5 | Icon color, back button, BarTextColor across modes | -| Shell/TabBar | 4 | TabBarIsVisible, Shell crashes, section rendering | - -Use this table as a triage guide: PRs touching these components warrant a more thorough pass through sections 1–21 above, with particular attention to the adjacent-scenario rule (§21 row 3) and the component-specific rows in this section. - ---- - -## What NOT to Flag - -Do not waste reviewer time on these: - -| Category | Why | -|----------|-----| -| **Style/formatting** | CI enforces via `dotnet format`. | -| **Missing XML docs on non-public APIs** | Not required by MAUI convention. | -| **Test naming preferences** | Unless names are genuinely misleading. | -| **`var` vs explicit types** | Project allows both; consistency within a file is sufficient. | -| **Micro-optimizations in cold paths** | Readability wins unless profiling proves it's a hot path. | -| **Single-use LINQ vs foreach** | Either is fine; don't bikeshed. | -| **Comment style** | Only flag if a comment is factually wrong or stale. | -| **PR commit count/squash** | That's the author's workflow choice. | diff --git a/.github/skills/code-review/tests/eval.yaml b/.github/skills/code-review/tests/eval.yaml index 8e0d8e981045..f02d9d1ad109 100644 --- a/.github/skills/code-review/tests/eval.yaml +++ b/.github/skills/code-review/tests/eval.yaml @@ -26,7 +26,7 @@ scenarios: value: "Devil's Advocate" rubric: - "The agent provides a plain summary without launching a structured multi-step review workflow" - - "The agent does NOT load review-rules.md or walk through MAUI-specific review checklists" + - "The agent does NOT walk through a multi-step review workflow" timeout: 120 - name: "Independence-first - agent reads diff before description" @@ -64,7 +64,7 @@ scenarios: pattern: "(NEEDS_CHANGES|NEEDS_DISCUSSION)" rubric: - "If the agent finds or confirms a ❌ Error-level issue, the verdict is NEEDS_CHANGES — not LGTM" - - "The agent applies handler lifecycle rules from review-rules.md (ConnectHandler/DisconnectHandler symmetry)" + - "The agent applies handler lifecycle rules from the expert reviewer dimensions (ConnectHandler/DisconnectHandler symmetry)" - "The agent cites specific file and line references for the concern" timeout: 300 diff --git a/.github/skills/pr-review/SKILL.md b/.github/skills/pr-review/SKILL.md index eab358b1d112..51baca74c8f3 100644 --- a/.github/skills/pr-review/SKILL.md +++ b/.github/skills/pr-review/SKILL.md @@ -244,10 +244,14 @@ CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/ ├── try-fix/ │ ├── content.md # Phase 2 summary │ └── attempt-{N}/ # Per-model attempt -│ ├── approach.md # What was tried -│ ├── result.txt # Pass / Fail / Blocked -│ ├── fix.diff # git diff of changes -│ └── analysis.md # Why it worked/failed +│ ├── baseline.log # Baseline establishment proof +│ ├── approach.md # What was tried +│ ├── result.txt # Pass / Fail / Blocked +│ ├── fix.diff # git diff of changes +│ ├── test-output.log # Full test command output +│ ├── reviewer-findings.json # Inline expert self-review (`[]` if clean) — reflects the FINAL diff (refreshed by Step 7.5 if test loop modified code) +│ ├── reviewer-findings.diff # Snapshot of the diff that the self-review evaluated (used by Step 7.5 to detect drift) +│ └── analysis.md # Why it worked/failed + self-review summary └── report/ └── content.md # Phase 3 output (pr-report) ``` diff --git a/.github/skills/try-fix/SKILL.md b/.github/skills/try-fix/SKILL.md index ca1fcdfc4089..e421eabffed5 100644 --- a/.github/skills/try-fix/SKILL.md +++ b/.github/skills/try-fix/SKILL.md @@ -26,7 +26,7 @@ If the prompt does not include a **problem to fix** and a **test command to veri 4. **Empirical** - Actually implement and test, don't just theorize 5. **Context-driven** - Work with what's provided and git history; don't search external sources -**Every invocation:** Review existing fixes → Think of DIFFERENT approach → Implement and test → Report results +**Every invocation runs all 11 Workflow steps below.** Step 6 (Expert Self-Review) is performed inline against `.github/agents/maui-expert-reviewer.md` — do NOT spawn the `@maui-expert-reviewer` sub-agent. Step 7.5 refreshes the self-review if the test loop modified code so the recorded findings reflect the final diff. Step 8 enforces this via a file-existence gate on `reviewer-findings.json`. ## ⚠️ CRITICAL: Sequential Execution Only @@ -69,6 +69,7 @@ Results reported back to the invoker: | `result` | `Pass`, `Fail`, or `Blocked` | | `analysis` | Why it worked, or why it failed and what was learned | | `diff` | The actual code changes made (for review) | +| `findings_count` | Number of self-review findings recorded (0 = clean self-review) | ## Output Structure (MANDATORY) @@ -96,10 +97,12 @@ Write-Host "Output directory: $OUTPUT_DIR" |------|----------------|---------| | `baseline.log` | After Step 2 (Baseline) | Output from EstablishBrokenBaseline.ps1 proving baseline was established | | `approach.md` | After Step 4 (Design) | What fix you're attempting and why it's different from existing fixes | -| `result.txt` | After Step 6 (Test) | Single word: `Pass`, `Fail`, or `Blocked` | -| `fix.diff` | After Step 6 (Test) | Output of `git diff` showing your changes | -| `test-output.log` | After Step 6 (Test) | Full output from test command | -| `analysis.md` | After Step 6 (Test) | Why it worked/failed, insights learned | +| `reviewer-findings.json` | After Step 6 (Self-Review), refreshed by Step 7.5 | JSON array of self-review findings — `[]` when clean. **MUST reflect the final diff.** | +| `reviewer-findings.diff` | After Step 6 (Self-Review), refreshed by Step 7.5 | Snapshot of `git diff` at the time the self-review was written. Step 7.5 compares this to the post-test-loop diff to detect drift. | +| `result.txt` | After Step 7 (Test) | Single word: `Pass`, `Fail`, or `Blocked` | +| `fix.diff` | After Step 7 (Test) | Output of `git diff` showing your changes | +| `test-output.log` | After Step 7 (Test) | Full output from test command | +| `analysis.md` | After Step 8 (Capture) | Why it worked/failed, insights learned, and a one-line self-review summary | **Example approach.md:** ```markdown @@ -125,10 +128,11 @@ The skill is complete when: - [ ] ONE fix approach designed and implemented - [ ] Fix tested with provided test command (iterated up to 3 times if errors/failures) - [ ] Either: Tests PASS ✅, or exhausted attempts and documented why approach won't work ❌ +- [ ] **Expert self-review performed inline (Step 6) and `reviewer-findings.json` written** — `[]` if clean. **Refreshed by Step 7.5 if the test loop modified code, so the saved findings reflect the final diff.** - [ ] Analysis provided (success explanation or failure reasoning with evidence) -- [ ] Artifacts saved to output directory +- [ ] Artifacts saved to output directory (verified by Step 8 file-existence gate) - [ ] Baseline restored (working directory clean) -- [ ] Results reported to invoker +- [ ] Results reported to invoker (including `findings_count`) 🚨 **CRITICAL: What counts as "Pass" vs "Fail"** @@ -261,7 +265,86 @@ Based on your analysis and any provided hints, design a single fix approach: Implement your fix. Use `git status --short` and `git diff` to track changes. -### Step 6: Test and Iterate (MANDATORY) +### Step 6: Expert Self-Review (MANDATORY — runs BEFORE testing) + +🚨 **You perform this self-review yourself. Do NOT spawn the `@maui-expert-reviewer` sub-agent.** Step 8's file-existence gate enforces that `reviewer-findings.json` is written every attempt. + +This step runs BEFORE testing so you can catch design flaws before spending time on build+test cycles. + +**Procedure:** + +1. **Read the rules.** View these specific sections of `.github/agents/maui-expert-reviewer.md`: + - **`## Overarching Principles`** (8 numbered principles, near the top of the file) — apply to every fix + - **`## Dimension Routing`** + **`### Always-Active Dimensions`** — pick the dimensions that match your changed files + - For each routed dimension, jump to its CHECK list under `## Review Dimensions` (e.g., `### 1. Layout Measure-Arrange Correctness`) + + You only need the dimensions that match the files you actually touched plus the always-active ones — typically 3–6 sections, not all 30. + +2. **Identify your changed files:** + ```powershell + git diff --name-only HEAD + ``` + + If you have NO code changes (e.g., Blocked because no device available before any fix was applied), still proceed to step 4 and write `'[]'` — the artifact gate is the enforcement mechanism. + +3. **Walk your diff against the rules:** + - For each Overarching Principle → does your diff violate it? + - For each routed dimension → walk every CHECK rule against the relevant hunks + - Always-Active dimensions (Logic and Correctness, Regression Prevention, Complexity Reduction) → apply regardless of file paths + - Be honest. If unsure, flag it. + +4. **Write findings to `$OUTPUT_DIR/reviewer-findings.json`.** Always write the file, even when there are zero findings. Use the same JSON format as the `@maui-expert-reviewer` agent (matches the GitHub Pull Request Review API): + + ```powershell + # No findings — clean self-review (or no diff to review): + '[]' | Set-Content "$OUTPUT_DIR/reviewer-findings.json" + + # With findings — JSON array of {path, line, body}: + @' + [ + { + "path": "src/Core/src/Handlers/ScrollView/ScrollViewHandler.iOS.cs", + "line": 42, + "body": "**[major] Layout Measure-Arrange** — Content measured with unconstrained height but arranged with bounded height. Concrete scenario: ScrollView inside a Grid with Star row height." + } + ] + '@ | Set-Content "$OUTPUT_DIR/reviewer-findings.json" + ``` + + Each entry has exactly 3 fields: + - **`path`** (string) — file relative to repo root, must be a file present in your diff + - **`line`** (integer ≥ 1) — line number on the changed (right) side of the diff. The line MUST appear in your diff — picking an unchanged line is wrong. Use `1` only as a fallback for file-level concerns where no single line captures the issue (e.g., missing import, structural concern). + - **`body`** (string) — format `**[severity] Dimension** — description`. Severity is one of `critical`/`major`/`moderate`/`minor`. + +5. **Validate the JSON parses and capture the count:** + ```powershell + try { + $findings = @(Get-Content "$OUTPUT_DIR/reviewer-findings.json" -Raw | ConvertFrom-Json) + $findingsCount = $findings.Count + Write-Host "✅ reviewer-findings.json: $findingsCount findings" + } catch { + Write-Host "❌ reviewer-findings.json is invalid JSON: $_" + throw + } + + # Snapshot the diff that was reviewed — Step 7.5 uses this to detect whether the test loop mutated code. + # Use Set-Content -Value with Out-String so the file is created even when the diff is empty + # (a bare `git diff | Set-Content` does NOT create the file when the pipe is empty). + Set-Content -Path "$OUTPUT_DIR/reviewer-findings.diff" -Value (git diff | Out-String) -NoNewline + ``` + + **Remember `$findingsCount`** — you will report it as `findings_count` in Step 10 and summarize it in `analysis.md` (Step 8). + +6. **Fix critical/major findings BEFORE testing:** + - If there are any `[critical]` or `[major]` findings → apply fixes for them in a single batch and rewrite `reviewer-findings.json` to reflect the new diff. + - All `[moderate]` and `[minor]` findings → note in `analysis.md` (Step 8); do NOT iterate. + - Only ONE correction round. Then proceed to Step 7 (Test). + +**Threshold guidance.** Only record findings with a concrete failing scenario. Stylistic preferences and bikeshedding (see the **`## What NOT to Flag`** table in `maui-expert-reviewer.md`) are not findings. An empty `[]` is the correct output for a clean fix — do not invent findings to fill the file. + +> **Why before testing?** Self-review catches design flaws (wrong null check, missing platform guard, thread safety issue) before you spend 5-15 minutes on a build+test cycle. It also runs when context is lightest — before test output floods the context window. + +### Step 7: Test and Iterate (MANDATORY) 🚨 **CRITICAL: ALWAYS use the provided test command script - NEVER manually build/compile.** @@ -286,7 +369,7 @@ pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project **Why no programmatic "did you actually rewrite the JSON" check?** A SHA256 hash sentinel rejects the legitimate byte-identical case (e.g., `[]` → `[]` after a small compile fix that introduces no new violations), and that case is common. The procedural enforcement is sub-step 2's explicit numbered list above, plus the example-invocation chain that walks the dimensions explicitly. If sub-step 2 is skipped, the JSON validates but ships a stale review — accept that risk in exchange for not blocking valid clean fixes. + +**Severity handling is the same as Step 6.** If the refresh surfaces new `[critical]` or `[major]` findings, you may apply ONE more fix batch and re-run the test loop, then re-refresh. Do not loop indefinitely — if a fix introduces critical findings on the third pass, mark the attempt `Blocked` and explain in `analysis.md`. + +### Step 8: Capture Artifacts (MANDATORY) **Before reverting, save ALL required files to `$OUTPUT_DIR`:** @@ -307,13 +446,18 @@ See [references/compile-errors.md](references/compile-errors.md) for error patte # 1. Save result (MUST be exactly "Pass", "Fail", or "Blocked") "Pass" | Set-Content "$OUTPUT_DIR/result.txt" # or "Fail" -# 2. Save the diff -git diff | Set-Content "$OUTPUT_DIR/fix.diff" +# 2. Save the diff (use Set-Content -Value with Out-String so the file is created +# even when the diff is empty — a bare `git diff | Set-Content` does not create +# the file when the pipe is empty, which would fail the artifact gate.) +Set-Content -Path "$OUTPUT_DIR/fix.diff" -Value (git diff | Out-String) -NoNewline -# 3. Save test output (should already exist from Step 6) +# 3. Save test output (should already exist from Step 7) # Copy-Item "path/to/test-output.log" "$OUTPUT_DIR/test-output.log" -# 4. Save analysis +# 4. reviewer-findings.json should already exist from Step 6 (and may have been refreshed by Step 7.5) +# 4b. reviewer-findings.diff snapshot (used by Step 7.5 to detect drift) + +# 5. Save analysis (include a one-line summary of self-review findings) @" ## Analysis @@ -323,23 +467,46 @@ git diff | Set-Content "$OUTPUT_DIR/fix.diff" **Why it worked/failed:** [Root cause analysis] +**Self-review:** [N findings: brief summary of each, or "clean — no findings"] + **Insights:** [What was learned that could help future attempts] "@ | Set-Content "$OUTPUT_DIR/analysis.md" ``` -**Verify all required files exist:** +**Verify all required files exist (this is the enforcement gate for Steps 6 and 7 — primarily `reviewer-findings.json` from Step 6, refreshed by Step 7.5 if needed):** + +🚨 **The artifact check below MUST be wrapped so that Step 9 (Restore) ALWAYS runs even if the check fails.** A failed gate that skips restore would leave the worktree dirty and corrupt the next sequential try-fix attempt. + ```powershell -@("baseline.log", "approach.md", "result.txt", "fix.diff", "analysis.md", "test-output.log") | ForEach-Object { - if (Test-Path "$OUTPUT_DIR/$_") { Write-Host "✅ $_" } - else { Write-Host "❌ MISSING: $_" } +# Run the file-existence check, but DEFER any throw until after Step 9 has restored the worktree. +$missing = @() +@("baseline.log", "approach.md", "result.txt", "fix.diff", "analysis.md", "test-output.log", "reviewer-findings.json", "reviewer-findings.diff") | ForEach-Object { + if (Test-Path "$OUTPUT_DIR/$_") { + Write-Host "✅ $_" + } else { + Write-Host "❌ MISSING: $_" + $missing += $_ + } +} + +# Record the gate result for use after Step 9 — DO NOT throw here. +if ($missing.Count -gt 0) { + $gateFailureMessage = "Required artifacts missing: $($missing -join ', '). If 'reviewer-findings.json' is missing, Step 6 (Expert Self-Review) was not performed (or Step 7.5 did not refresh it after the test loop) — it is mandatory and must contain at least '[]' that reflects the final diff." + Write-Host "⚠️ ARTIFACT GATE FAILED — proceeding to Step 9 restore before reporting failure." + Write-Host $gateFailureMessage + "Blocked" | Set-Content "$OUTPUT_DIR/result.txt" -Force +} else { + $gateFailureMessage = $null } ``` +**If `$gateFailureMessage` was set:** Step 9 still runs (do NOT skip it). After Step 9 restores the worktree, surface the failure in Step 10's report — set `result.txt` to `Blocked` (already done above) and explain in `analysis.md` which artifact was missing. The next sequential attempt then starts from a clean worktree. + **Analysis quality matters.** Bad: "Didn't work". Good: "Fix attempted to reset state in OnPageSelected, but this fires after layout measurement. The cached value was already used." -### Step 8: Restore Working Directory (MANDATORY) +### Step 9: Restore Working Directory (MANDATORY — runs even if Step 8 gate failed) -**ALWAYS restore, even if fix failed.** +**ALWAYS restore, even if fix failed or Step 8 detected missing artifacts.** Skipping restore corrupts the next sequential try-fix attempt. ```bash pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore @@ -347,7 +514,7 @@ pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore 🚨 Use `EstablishBrokenBaseline.ps1 -Restore` — not `git checkout`, `git restore`, or `git reset` (see Step 2 for why). -### Step 9: Report Results +### Step 10: Report Results Provide structured output to the invoker: @@ -361,6 +528,8 @@ Provide structured output to the invoker: **Result:** ✅ PASS / ❌ FAIL +**Self-Review:** N findings (X critical, Y major, Z moderate/minor) — see `reviewer-findings.json` + **Analysis:** [Why it worked, or why it failed and what was learned] @@ -381,7 +550,7 @@ Provide structured output to the invoker: | Test command fails to run | Report build/setup error with details | | Test times out | Report timeout, include partial output | | Can't determine fix approach | Report "no viable approach identified" with reasoning | -| Git state unrecoverable | Run `pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore` (see Step 2/8) | +| Git state unrecoverable | Run `pwsh .github/scripts/EstablishBrokenBaseline.ps1 -Restore` (see Step 2/9) | --- diff --git a/.github/skills/try-fix/references/example-invocation.md b/.github/skills/try-fix/references/example-invocation.md index 112f8b7ce069..362077a665c1 100644 --- a/.github/skills/try-fix/references/example-invocation.md +++ b/.github/skills/try-fix/references/example-invocation.md @@ -23,4 +23,4 @@ hints: | - Focus on the Disconnect/Cleanup methods ``` -**Skill execution:** Reads context → Analyzes target files → Designs fix (add IsDisposed check) → Applies fix → Runs test (PASS) → Reports result → Reverts changes +**Skill execution:** Reads context → Analyzes target files → Designs fix (add IsDisposed check) → Applies fix → Performs inline expert self-review against `.github/agents/maui-expert-reviewer.md` rules and writes `reviewer-findings.json` (`[]` if clean) → Runs test (PASS) → If code changed during the test loop, refreshes `reviewer-findings.json` against the final diff → Captures artifacts → Reverts changes → Reports result diff --git a/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 b/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 index 168f9211bf9e..c623c8a8860f 100644 --- a/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 +++ b/.github/skills/verify-tests-fail-without-fix/scripts/verify-tests-fail.ps1 @@ -622,8 +622,42 @@ function Get-TestResultFromOutput { } # Check for build failures (before any test results) + # Mark these explicitly with BuildError = $true so Write-MarkdownReport can + # surface them as "Fix does not compile" instead of "Fix does not pass the tests". if ($content -match "Build FAILED" -or $content -match "Build failed with exit code" -or $content -match "error MSB\d+" -or $content -match "error CS\d+") { - return @{ Passed = $false; Error = "Build failed before tests could run"; FailCount = 0; Failed = 0; Total = 0; Skipped = 0 } + # Capture the first compile error so the diagnosis is concrete. + $buildErrorExcerpt = $null + $errMatch = [regex]::Match($content, '(?m)^.*\b(error CS\d+|error MSB\d+)\b.*$') + if ($errMatch.Success) { + $excerpt = $errMatch.Value.Trim() + if ($excerpt.Length -gt 200) { $excerpt = $excerpt.Substring(0, 200) + "..." } + $buildErrorExcerpt = $excerpt + } + return @{ + Passed = $false; BuildError = $true + Error = if ($buildErrorExcerpt) { "Build failed: $buildErrorExcerpt" } else { "Build failed before tests could run" } + FailureMessage = $buildErrorExcerpt + FailCount = 0; Failed = 0; Total = 0; Skipped = 0 + } + } + + # Check for filter mismatch — the test runner ran successfully but the supplied + # -filter expression matched zero test cases. Without this branch the gate + # would treat "0 tests ran" as ENV ERROR (or worse, silently as a failed + # test) — both misclassifications. The fix is to surface this as a separate + # "FilterMismatch" outcome so Write-MarkdownReport can label it accurately. + if ($content -match 'No test matches the given testcase filter' -or + $content -match '(?im)^\s*Test count:\s*0\b') { + $attemptedFilter = $null + $fmMatch = [regex]::Match($content, "No test matches the given testcase filter '([^']+)'") + if ($fmMatch.Success) { $attemptedFilter = $fmMatch.Groups[1].Value } + elseif ($TestFilter) { $attemptedFilter = $TestFilter } + return @{ + Passed = $false; FilterMismatch = $true + Error = if ($attemptedFilter) { "Test filter '$attemptedFilter' matched 0 tests" } else { "Test filter matched 0 tests" } + FailureMessage = $attemptedFilter + FailCount = 0; Failed = 0; Total = 0; Skipped = 0 + } } # --- Device test output: [PASS]/[FAIL] markers from xharness --- @@ -1153,11 +1187,74 @@ function Write-MarkdownReport { $status = if ($hasEnvError) { "⚠️ ENV ERROR" } elseif ($VerificationPassed) { "✅ PASSED" } else { "❌ FAILED" } $mergeBaseShort = if ($ReportMergeBase -and $ReportMergeBase.Length -ge 8) { $ReportMergeBase.Substring(0, 8) } else { "$ReportMergeBase" } + # ─── Improvement #2: classify the failure mode so the headline matches the cause ─── + # Without this, every non-PASSED gate just says "tests did not behave as expected". + # Map the without/with-fix outcomes per test into a concrete diagnosis the + # downstream Try-Fix×4 stage and the human reader can act on. + # + # Reliability extensions: + # - BuildError flag → headline says "Fix does not compile" (was conflated + # with "Fix does not pass the tests" because the test runner can't load + # an assembly that doesn't compile, so every test in it appears to fail). + # - FilterMismatch flag → headline says "Test filter matched 0 tests" + # (was misclassified as ENV ERROR or as a generic FAIL because zero + # tests ran but exit code was non-zero). + $failureClassification = $null + if (-not $hasEnvError -and -not $VerificationPassed -and $WithoutFixResultsList -and $WithFixResultsList) { + # Build error in the with-fix run trumps every other classification — if + # the fix doesn't compile, no per-test outcome is meaningful. + $wBuildError = @($WithFixResultsList | Where-Object { $_.BuildError }) + $woBuildError = @($WithoutFixResultsList | Where-Object { $_.BuildError }) + $wFilterMiss = @($WithFixResultsList | Where-Object { $_.FilterMismatch }) + $woFilterMiss = @($WithoutFixResultsList | Where-Object { $_.FilterMismatch }) + + $woStates = @($WithoutFixResultsList | ForEach-Object { if ($_.EnvError) { "ENV" } elseif ($_.BuildError) { "BUILD" } elseif ($_.FilterMismatch) { "NOMATCH" } elseif ($_.Passed) { "PASS" } else { "FAIL" } }) + $wStates = @($WithFixResultsList | ForEach-Object { if ($_.EnvError) { "ENV" } elseif ($_.BuildError) { "BUILD" } elseif ($_.FilterMismatch) { "NOMATCH" } elseif ($_.Passed) { "PASS" } else { "FAIL" } }) + + $allWoPass = ($woStates | Where-Object { $_ -ne "PASS" }).Count -eq 0 + $allWoFail = ($woStates | Where-Object { $_ -ne "FAIL" }).Count -eq 0 + $allWFail = ($wStates | Where-Object { $_ -ne "FAIL" }).Count -eq 0 + $hasRegression = $false + # Regression: at least one test fixes (FAIL→PASS) AND at least one regresses (FAIL→FAIL) + for ($i = 0; $i -lt $woStates.Count -and $i -lt $wStates.Count; $i++) { + if ($woStates[$i] -eq "FAIL" -and $wStates[$i] -eq "FAIL") { $hasRegression = $true } + } + $hasFixedTest = $false + for ($i = 0; $i -lt $woStates.Count -and $i -lt $wStates.Count; $i++) { + if ($woStates[$i] -eq "FAIL" -and $wStates[$i] -eq "PASS") { $hasFixedTest = $true } + } + + if ($wBuildError.Count -gt 0) { + $excerpt = ($wBuildError | ForEach-Object { $_.FailureMessage } | Where-Object { $_ } | Select-Object -First 1) + $excerptLine = if ($excerpt) { "`n> ``$excerpt``" } else { "" } + $failureClassification = "🩺 **Fix does not compile** — applying the PR's fix produces a build error before tests can run. The earlier-than-test failure is the root cause; the per-test ❌ FAIL marks are downstream effects, not real test failures.$excerptLine" + } elseif ($woBuildError.Count -gt 0) { + $failureClassification = "🩺 **Base branch does not compile** — the without-fix build failed. The gate's ""does the test fail without the fix"" check is unreliable here; this usually means ``main`` is broken or a merge-base file went missing. Investigate before trusting this gate." + } elseif ($wFilterMiss.Count -gt 0 -or $woFilterMiss.Count -gt 0) { + $missing = ($wFilterMiss + $woFilterMiss | ForEach-Object { $_.FailureMessage } | Where-Object { $_ } | Select-Object -First 1) + $hint = if ($missing) { " — filter ``$missing`` matched 0 tests" } else { "" } + $failureClassification = "🩺 **Test filter mismatch**$hint. The test runner produced zero results because no test class or method matched the filter. Common causes: the gate filter was derived from the file name but the actual test class is named differently, or the test was renamed/moved without updating the auto-detection. Verify the test class name matches what the gate is searching for." + } elseif ($allWoPass) { + $failureClassification = "🩺 **Test does not reproduce the bug** — ran the same in both states (PASS without fix, PASS with fix). The repro test is not exercising the issue. Strengthen the test before reviewing the fix." + } elseif ($allWoFail -and $allWFail) { + $failureClassification = "🩺 **Fix does not pass the tests** — every test still fails after applying the fix. The PR's change does not resolve the failure(s)." + } elseif ($hasFixedTest -and $hasRegression) { + $failureClassification = "🩺 **Regression in another test** — at least one test goes FAIL→PASS (fix works there), but another test FAILs both with and without the fix. The fix breaks a pre-existing or sibling test." + } elseif ($hasRegression -and -not $hasFixedTest) { + $failureClassification = "🩺 **Fix breaks tests** — one or more tests fail with the fix applied, and none of the failures are resolved by the fix." + } + # else: leave $failureClassification unset; the per-test table + Failure Details below tell the story. + } + $lines = @() $lines += "### Gate Result: $status" $lines += "" $platformDisplay = if ($ReportPlatform) { $ReportPlatform.ToUpper() } else { "N/A" } $lines += "**Platform:** $platformDisplay · **Base:** $ReportBaseBranch · **Merge base:** ``$mergeBaseShort``" + if ($failureClassification) { + $lines += "" + $lines += $failureClassification + } $lines += "" # ── Side-by-side per-test comparison table ── @@ -1172,6 +1269,10 @@ function Write-MarkdownReport { $woDur = if ($woResult.Duration) { "$([math]::Round($woResult.Duration.TotalSeconds))s" } else { "" } if ($woResult.EnvError) { $woCell = "⚠️ ENV ERROR" + } elseif ($woResult.BuildError) { + $woCell = "🛠️ BUILD ERROR" + } elseif ($woResult.FilterMismatch) { + $woCell = "🔍 NO MATCH" } elseif (-not $woResult.Passed) { $woCell = "✅ FAIL — $woDur" } else { @@ -1182,6 +1283,10 @@ function Write-MarkdownReport { $wDur = if ($wResult.Duration) { "$([math]::Round($wResult.Duration.TotalSeconds))s" } else { "" } if ($wResult.EnvError) { $wCell = "⚠️ ENV ERROR" + } elseif ($wResult.BuildError) { + $wCell = "🛠️ BUILD ERROR" + } elseif ($wResult.FilterMismatch) { + $wCell = "🔍 NO MATCH" } elseif ($wResult.Passed) { $wCell = "✅ PASS — $wDur" } else { @@ -1203,7 +1308,7 @@ function Write-MarkdownReport { # Without fix log $woLogFile = Join-Path $OutputPath "test-without-fix-$sanitizedName.log" - $woStatus = if ($woResult.EnvError) { "⚠️ ENV ERROR" } elseif (-not $woResult.Passed) { "FAIL ✅" } else { "PASS ❌" } + $woStatus = if ($woResult.EnvError) { "⚠️ ENV ERROR" } elseif ($woResult.BuildError) { "🛠️ BUILD ERROR" } elseif ($woResult.FilterMismatch) { "🔍 NO MATCH" } elseif (-not $woResult.Passed) { "FAIL ✅" } else { "PASS ❌" } $woDur = if ($woResult.Duration) { " · $([math]::Round($woResult.Duration.TotalSeconds))s" } else { "" } $lines += "" $lines += "
" @@ -1232,7 +1337,7 @@ function Write-MarkdownReport { # With fix log $wLogFile = Join-Path $OutputPath "test-with-fix-$sanitizedName.log" - $wStatus = if ($wResult.EnvError) { "⚠️ ENV ERROR" } elseif ($wResult.Passed) { "PASS ✅" } else { "FAIL ❌" } + $wStatus = if ($wResult.EnvError) { "⚠️ ENV ERROR" } elseif ($wResult.BuildError) { "🛠️ BUILD ERROR" } elseif ($wResult.FilterMismatch) { "🔍 NO MATCH" } elseif ($wResult.Passed) { "PASS ✅" } else { "FAIL ❌" } $wDur = if ($wResult.Duration) { " · $([math]::Round($wResult.Duration.TotalSeconds))s" } else { "" } $lines += "" $lines += "
" @@ -1262,13 +1367,31 @@ function Write-MarkdownReport { # ── Failure details (shown directly — not collapsed) ── $failureLines = @() foreach ($r in $WithoutFixResultsList) { - if ($r.Passed) { + if ($r.BuildError) { + $failureLines += "- 🛠️ **$($r.TestName)** without fix: build failed before tests could run" + if ($r.FailureMessage) { + $msg = if ($r.FailureMessage.Length -gt 300) { $r.FailureMessage.Substring(0, 300) + "..." } else { $r.FailureMessage } + $failureLines += " - ``$msg``" + } + } elseif ($r.FilterMismatch) { + $failureLines += "- 🔍 **$($r.TestName)** without fix: test filter matched 0 tests" + if ($r.FailureMessage) { $failureLines += " - filter: ``$($r.FailureMessage)``" } + } elseif ($r.Passed) { $failureLines += "- ❌ **$($r.TestName)** PASSED without fix (should fail) — tests don't catch the bug" } if ($r.EnvError) { $failureLines += "- ⚠️ **$($r.TestName)** without fix: ``$($r.Error)``" } } foreach ($r in $WithFixResultsList) { - if (-not $r.Passed -and -not $r.EnvError) { + if ($r.BuildError) { + $failureLines += "- 🛠️ **$($r.TestName)** with fix: build failed (fix does not compile)" + if ($r.FailureMessage) { + $msg = if ($r.FailureMessage.Length -gt 300) { $r.FailureMessage.Substring(0, 300) + "..." } else { $r.FailureMessage } + $failureLines += " - ``$msg``" + } + } elseif ($r.FilterMismatch) { + $failureLines += "- 🔍 **$($r.TestName)** with fix: test filter matched 0 tests" + if ($r.FailureMessage) { $failureLines += " - filter: ``$($r.FailureMessage)``" } + } elseif (-not $r.Passed -and -not $r.EnvError) { $failureLines += "- ❌ **$($r.TestName)** FAILED with fix (should pass)" if ($r.FailureReason) { $failureLines += " - ``$($r.FailureReason)``" } if ($r.FailureMessage) { @@ -1280,10 +1403,10 @@ function Write-MarkdownReport { } if ($failureLines.Count -gt 0) { - # Count actual failed tests (lines beginning with "- ❌" or "- ⚠️") to decide - # whether to collapse. Sub-bullets (FailureReason / FailureMessage) start with - # two leading spaces so they don't match. - $failedTestCount = @($failureLines | Where-Object { $_ -match '^- (❌|⚠️)' }).Count + # Count actual failed tests (lines beginning with "- ❌"/"- ⚠️"/"- 🛠️"/"- 🔍") + # to decide whether to collapse. Sub-bullets (FailureReason / FailureMessage) + # start with two leading spaces so they don't match. + $failedTestCount = @($failureLines | Where-Object { $_ -match '^- (❌|⚠️|🛠️|🔍)' }).Count # Threshold: if more than 5 tests failed, collapse the section so the gate # summary stays visible above the fold in PR comments. Below the threshold, # show details inline so reviewers don't need an extra click. diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index e7376bb48184..acff08c44cb7 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -166,17 +166,17 @@ https://github.com/dotnet/dotnet 7b29526f2107416f68578bcb9deaca74fcfcf7f0 - + https://github.com/dotnet/xharness - 9d5a7e96236317f6312b25590e93ce6c363b62c3 + 92962e5c46ac08a66ded4c5696209cc60f1a232f - + https://github.com/dotnet/xharness - 9d5a7e96236317f6312b25590e93ce6c363b62c3 + 92962e5c46ac08a66ded4c5696209cc60f1a232f - + https://github.com/dotnet/xharness - 9d5a7e96236317f6312b25590e93ce6c363b62c3 + 92962e5c46ac08a66ded4c5696209cc60f1a232f diff --git a/eng/Versions.props b/eng/Versions.props index 4f97597832f3..db665fa0fa24 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -137,9 +137,9 @@ <_HarfBuzzSharpVersion>8.3.0.1 <_SkiaSharpNativeAssetsVersion>0.0.0-commit.e57e2a11dac4ccc72bea52939dede49816842005.1728 7.0.120 - 11.0.0-prerelease.26229.1 - 11.0.0-prerelease.26229.1 - 11.0.0-prerelease.26229.1 + 11.0.0-prerelease.26230.4 + 11.0.0-prerelease.26230.4 + 11.0.0-prerelease.26230.4 0.9.2 2.0.0.4 1.3.0 diff --git a/eng/pipelines/ci-copilot.yml b/eng/pipelines/ci-copilot.yml index 165cb94cd6c0..2f77543be29d 100644 --- a/eng/pipelines/ci-copilot.yml +++ b/eng/pipelines/ci-copilot.yml @@ -649,6 +649,7 @@ stages: COPILOT_GITHUB_TOKEN: $(COPILOT_TOKEN) GH_TOKEN: $(GH_COMMENT_TOKEN) DEVICE_UDID: $(DEVICE_UDID) + COMMENTS_VIA_FILE: "true" # Publish Copilot logs and session artifacts - task: PublishPipelineArtifact@1 diff --git a/src/Core/src/Platform/Android/ActivityIndicatorExtensions.cs b/src/Core/src/Platform/Android/ActivityIndicatorExtensions.cs index 9bc649216c45..d3dc568c6202 100644 --- a/src/Core/src/Platform/Android/ActivityIndicatorExtensions.cs +++ b/src/Core/src/Platform/Android/ActivityIndicatorExtensions.cs @@ -7,7 +7,35 @@ public static class ActivityIndicatorExtensions { public static void UpdateIsRunning(this ProgressBar progressBar, IActivityIndicator activityIndicator) { - progressBar.Visibility = GetActivityIndicatorVisibility(activityIndicator); + // Guard: check IsDisposed() before accessing any view properties to avoid ObjectDisposedException + // on a ProgressBar whose native handle has already been released (see ViewExtensions pattern). + if (progressBar.IsDisposed()) + { + return; + } + + // Pre-compute the desired visibility from the current activityIndicator state before deferring, + // so the lambda performs a simple write with no risk of reading stale handler state. + // Defer via Post() only when a layout traversal is in progress OR the view is not yet attached + // (RecyclerView header collapse scenario, see https://github.com/dotnet/maui/issues/33780). + // On the common path (attached, idle), apply synchronously so that existing tests and callers + // that read state immediately after update continue to work correctly. + if (progressBar.IsInLayout || !progressBar.IsAttachedToWindow) + { + var targetVisibility = GetActivityIndicatorVisibility(activityIndicator); + progressBar.Post(() => + { + // Guard: skip if the view was recycled/disposed or detached before the runnable fired. + if (!progressBar.IsDisposed()) + { + progressBar.Visibility = targetVisibility; + } + }); + } + else + { + progressBar.Visibility = GetActivityIndicatorVisibility(activityIndicator); + } } internal static ViewStates GetActivityIndicatorVisibility(this IActivityIndicator activityIndicator) diff --git a/src/Templates/src/templates/maui-aspire-servicedefaults/MauiAspire.1.ServiceDefaults.csproj b/src/Templates/src/templates/maui-aspire-servicedefaults/MauiAspire.1.ServiceDefaults.csproj index d011350b2f8d..051e316564d0 100644 --- a/src/Templates/src/templates/maui-aspire-servicedefaults/MauiAspire.1.ServiceDefaults.csproj +++ b/src/Templates/src/templates/maui-aspire-servicedefaults/MauiAspire.1.ServiceDefaults.csproj @@ -11,9 +11,9 @@ - - - - + + + + diff --git a/src/TestUtils/src/VisualTestUtils.MagickNet/VisualTestUtils.MagickNet.csproj b/src/TestUtils/src/VisualTestUtils.MagickNet/VisualTestUtils.MagickNet.csproj index 7f1abf940168..220a50e160b9 100644 --- a/src/TestUtils/src/VisualTestUtils.MagickNet/VisualTestUtils.MagickNet.csproj +++ b/src/TestUtils/src/VisualTestUtils.MagickNet/VisualTestUtils.MagickNet.csproj @@ -6,7 +6,7 @@ - +