diff --git a/.github/agents/maui-expert-reviewer.md b/.github/agents/maui-expert-reviewer.md new file mode 100644 index 000000000000..adb91b7da0db --- /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 `inline-findings.json` 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 `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 `inline-findings.json`** — every finding that can be associated with a file+line goes here. Try hard to associate feedback to a specific location. + +Write to `CustomAgentLogsTmp/PRState/{PR}/PRAgent/inline-findings.json`: + +```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 `inline-findings.json`. 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..41385f7c4d24 --- /dev/null +++ b/.github/instructions/performance-hotpaths.instructions.md @@ -0,0 +1,24 @@ +--- +applyTo: + - "src/Core/src/Layouts/**" + - "src/Controls/src/Core/Handlers/**" +--- +# Performance-Critical Path Rules + +## 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..f4ff83c9292f --- /dev/null +++ b/.github/instructions/public-api.instructions.md @@ -0,0 +1,25 @@ +--- +applyTo: + - "**/PublicAPI.Unshipped.txt" +--- +# Public API Surface Design + +## 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 b4f43982bae3..97e36d4b461c 100644 --- a/.github/scripts/Review-PR.ps1 +++ b/.github/scripts/Review-PR.ps1 @@ -556,7 +556,8 @@ $gateStatusForPrompt = switch ($gateResult) { } $step2Prompt = @" -Use a skill to review PR #$PRNumber. +First, use the code-review skill for dimensional code analysis with the maui-expert-reviewer agent — this writes inline-findings.json. +Then, use the pr-review skill to review PR #$PRNumber. The pr-review Report phase should account for any code quality issues found by the expert reviewer. $platformInstruction $autonomousRules @@ -565,6 +566,7 @@ $autonomousRules Do NOT re-run gate verification. The gate phase is handled separately. 📁 Write phase output to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/{phase}/content.md`` +📁 Also write inline review findings to ``CustomAgentLogsTmp/PRState/$PRNumber/PRAgent/inline-findings.json`` "@ Invoke-CopilotStep -StepName "STEP 2: PR REVIEW" -Prompt $step2Prompt | Out-Null @@ -609,6 +611,27 @@ if (Test-Path $reviewScript) { Write-Host " ⚠️ post-ai-summary-comment.ps1 not found — skipping review summary" -ForegroundColor Yellow } +# 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 + } +} + # ═════════════════════════════════════════════════════════════════════════════ # 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..292558b16298 --- /dev/null +++ b/.github/scripts/post-inline-review.ps1 @@ -0,0 +1,185 @@ +#!/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) { + $comment = @{ + path = $f.path + line = [int]$f.line + body = $f.body + } + # GitHub API requires 'side' for pull request review comments + $comment['side'] = 'RIGHT' + $comments += $comment +} + +$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/skills/code-review/SKILL.md b/.github/skills/code-review/SKILL.md index ab07ead11c2f..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 @@ -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 fa17c664d410..eab358b1d112 100644 --- a/.github/skills/pr-review/SKILL.md +++ b/.github/skills/pr-review/SKILL.md @@ -18,7 +18,7 @@ End-to-end PR review workflow that orchestrates phases to explore independent fi ``` Gate (pre-run) → Already completed by Review-PR.ps1 before this skill runs -Phase 1: Pre-Flight → Gather context, classify files → .github/pr-review/pr-preflight.md +Phase 1: Pre-Flight → Gather context, classify files, code review → .github/pr-review/pr-preflight.md Phase 2: Try-Fix → ⚠️ MANDATORY multi-model exploration → invoke try-fix skill (×4 models) Phase 3: Report → Write review recommendation → .github/pr-review/pr-report.md ``` @@ -26,6 +26,7 @@ Phase 3: Report → Write review recommendation → .g > **Gate and Branch setup** are handled by `Review-PR.ps1` before this skill is invoked. The gate result is passed in the prompt. Do NOT re-run gate verification. **All phases write output to:** `CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/{phase}/content.md` +**Pre-Flight also writes:** `CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/pre-flight/code-review.md` --- @@ -44,7 +45,7 @@ Phase 3: Report → Write review recommendation → .g ### Multi-Model Configuration -Phase 3 uses these 4 AI models (run SEQUENTIALLY — they modify the same files): +Phase 2 uses these 4 AI models (run SEQUENTIALLY — they modify the same files): | Order | Model | |-------|-------| @@ -71,10 +72,20 @@ Phase 3 uses these 4 AI models (run SEQUENTIALLY — they modify the same files) > Read and follow `.github/pr-review/pr-preflight.md` -Gather context from the issue, PR, comments, and classify changed files. +Gather context from the issue, PR, comments, classify changed files, and **perform a deep code review** using the `code-review` skill. + +Pre-Flight now has two parts: +- **Part A (Steps 1–6):** Context gathering — read issue, PR, comments, classify files +- **Part B (Step 7):** Code review — independence-first code analysis using `.github/skills/code-review/SKILL.md` and `.github/skills/code-review/references/review-rules.md` + +**Outputs:** +- `pre-flight/content.md` — Context + code review summary +- `pre-flight/code-review.md` — Full code-review output (findings, blast radius, failure-mode probes, verdict) **Gate:** None — always runs. +**Why code review runs here:** The code-review findings (❌ Errors, ⚠️ Warnings, failure-mode probes, blast radius) become **structured hints for Phase 2 (Try-Fix)**. Instead of each model starting from scratch, they receive concrete code concerns to address, leading to higher-quality fix exploration. + --- ## Phase 2: Try-Fix → Invoke `try-fix` Skill (×4 Models) @@ -87,6 +98,8 @@ Even if the PR's fix looks correct and Gate passed, you MUST still run all 4 mod ### 🚨 CRITICAL: try-fix is Independent of PR's Fix +"Independent" means each model explores a **different fix approach** from the PR's fix — not that models are isolated from code-review context. Code-review findings are provided as advisory background to improve fix quality. + The purpose is NOT to re-test the PR's fix, but to: 1. **Generate independent fix ideas** — What would YOU do to fix this bug? 2. **Test those ideas empirically** — Actually implement and run tests @@ -119,10 +132,27 @@ prompt: | - target_files: - src/{area}/{file1}.cs - src/{area}/{file2}.cs + - hints: | + Code review found the following concerns (advisory — use to inform your approach, not as a checklist): + Errors: + - {❌ Error finding 1 with file:line reference} + # Include warnings ONLY if relevant to the root cause: + # Warnings: + # - {⚠️ Warning — omit if unrelated to root cause} + Failure modes: + - {Failure mode 1}: {What happens in this scenario} + Blast radius: {Summary — e.g., "Runs for ALL toolbar items at startup, not just badged ones"} + Code review verdict: {LGTM / NEEDS_CHANGES / NEEDS_DISCUSSION} (confidence: {high/medium/low}) Generate ONE independent fix idea. Review the PR's fix first to ensure your approach is DIFFERENT. + "Independent" means exploring a different fix approach — the code review context above is background + information to help you make better decisions, not a constraint on your exploration. ``` +**Include code review context in the `hints` field** (try-fix's documented optional input). If Pre-Flight code review found no issues, use `hints: "Code review found no issues (verdict: LGTM)"`. If code review was SKIPPED, omit the `hints` field entirely. + +**Selectivity:** Only include ❌ Error findings and failure-mode probes that are relevant to the bug being fixed. Omit 💡 Suggestions. Include ⚠️ Warnings only if directly related to the root cause. + **Wait for each to complete before starting the next.** **🧹 MANDATORY: Clean up between attempts:** @@ -198,7 +228,7 @@ Deliver the final review recommendation. > 🚨 **DO NOT post any comments.** All output goes to `CustomAgentLogsTmp/PRState/`. -**Gate:** Phases 1-3 must be complete. +**Gate:** Phases 1-2 must be complete. --- @@ -207,18 +237,19 @@ Deliver the final review recommendation. ``` CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/ ├── pre-flight/ -│ └── content.md # Phase 1 output (pr-preflight) +│ ├── content.md # Phase 1 output (context + code review summary) +│ └── code-review.md # Full code-review skill output (findings, blast radius, verdict) ├── gate/ -│ └── content.md # Phase 2 output (pr-gate) +│ └── content.md # Gate output (pr-gate, run separately) ├── try-fix/ -│ ├── content.md # Phase 3 summary +│ ├── 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 └── report/ - └── content.md # Phase 4 output (pr-report) + └── content.md # Phase 3 output (pr-report) ``` --- @@ -227,10 +258,10 @@ CustomAgentLogsTmp/PRState/{PRNumber}/PRAgent/ | Phase | Instructions | Key Action | If Blocked | |-------|--------------|------------|------------| -| 1. Pre-Flight | `pr-preflight.md` | Read issue + PR context | Skip missing info, continue | -| 2. Gate | `pr-gate.md` | Verify tests via task agent | Document, continue to Try-Fix | -| 3. Try-Fix | `try-fix` skill (×4) | **4-model exploration (MANDATORY)** | Skip failing models, continue | -| 4. Report | `pr-report.md` | Write review recommendation | Never skip | +| Gate (pre-run) | `pr-gate.md` | Verify tests (run by Review-PR.ps1) | Result passed in prompt — if missing, document and continue | +| 1. Pre-Flight | `pr-preflight.md` | Read issue + PR context + **code review** | Skip missing info; if code review fails, set verdict to SKIPPED | +| 2. Try-Fix | `try-fix` skill (×4) | **4-model exploration with code-review hints (MANDATORY)** | Skip failing models, continue | +| 3. Report | `pr-report.md` | Write review recommendation | Never skip | --- diff --git a/.github/skills/try-fix/SKILL.md b/.github/skills/try-fix/SKILL.md index ca1fcdfc4089..a6e9df19d8e3 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:** Review existing fixes → Think of DIFFERENT approach → Implement and test → Invoke @maui-expert-reviewer and ask it to return all findings to you (do not post or write to .json). Fix every violation it reports, then re-run your tests to confirm build and tests still pass → Report results ## ⚠️ CRITICAL: Sequential Execution Only 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