ViewHandler: Skip null ToolTip initialization - #37842
Conversation
Avoid the platform ToolTip update during initial handler connection when the virtual view has the default null value. Reconnects and dynamic updates retain the existing behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5a03e13d-1a41-4652-a8c6-099eeb79cd50
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37842Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37842" |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the core ViewHandler property-mapper pipeline to avoid performing a platform ToolTip update during the initial handler connection when the virtual view’s ToolTip is the default null, while still allowing reconnects and subsequent updates to clear or apply tooltip state as needed.
Changes:
- Short-circuits
MapToolTipduring initial handler connection whenIToolTipElement.ToolTipisnull. - Preserves existing behavior for reconnect scenarios (where a reused platform view may need stale tooltip state cleared) and for dynamic updates (null/non-null transitions).
This comment has been minimized.
This comment has been minimized.
|
/azp run |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@StephaneDelcroix — new AI review results are available based on commit
43b67e6.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ⚠️ SKIPPED
No tests were detected in this PR.
Recommendation: Add tests to verify the fix using the write-tests-agent.
📋 Pre-Flight — Context & Validation
PR #37842 Pre-Flight
Context
- Title:
ViewHandler: Skip null ToolTip initialization - Base / head:
net11.0/stephanedelcroix/layout-perf-tooltip - Materialized review commit:
186863b5b8a6f1226517792939f656bc1239a6f4 - Problem: Initial handler connection maps every
IToolTipElement.ToolTip, including the defaultnullvalue. On iOS this reachesUIView.UpdateToolTip, which performs native tooltip lookup/update work that is unnecessary for a newly created platform view. - Required behavior: Avoid unnecessary initial null-tooltip work without changing non-null initialization, later null/non-null property updates, or reconnect behavior that may need to clear stale native tooltip state.
- Gate: Skipped because the PR adds no tests. Do not rerun the gate and do not create or overwrite
gate/content.md.
Existing PR Approach
The PR changes only src/Core/src/Handlers/View/ViewHandler.cs. In MapToolTip, it returns before handler.ToPlatform().UpdateToolTip(...) when both conditions hold:
handler.IsConnectingHandler() && tooltipContainer.ToolTip is nullThis follows existing default-value connect guards in MapAutomationId, MapClip, and MapShadow. Because IsConnectingHandler() excludes reconnect state, reconnects and dynamic null updates still reach the platform extension.
Direct Diff
@@ -655,7 +655,14 @@ namespace Microsoft.Maui.Handlers
{
#if PLATFORM
if (view is IToolTipElement tooltipContainer)
+ {
+ if (handler.IsConnectingHandler() && tooltipContainer.ToolTip is null)
+ {
+ return;
+ }
+
handler.ToPlatform().UpdateToolTip(tooltipContainer.ToolTip);
+ }
#endif
}Candidate Constraints
- Produce one alternative mechanism per attempt; do not merely relocate or restyle the PR's connect-time null guard.
- The baseline restoration allow-list is expected to contain only
src/Core/src/Handlers/View/ViewHandler.cs. If.github/.baseline-state.jsonsays otherwise, obey it exactly. - Preserve mapper customization, first-connect non-null application, reconnect clearing, and dynamic null/non-null updates.
- Platform: iOS.
- Pre-existing unrelated worktree changes under
.github/andeng/are harness-owned and must remain untouched.
Targeted Validation
No PR-added primary test was detected and no additional mandatory regression command was supplied. Use the smallest existing relevant iOS device-test category:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Core -Platform ios -TestFilter "Category=View"This includes the existing ViewHandlerTests tooltip cases for Label, Button, Image, and CheckBox with both non-null and null tooltip content. Run this command only; do not run a full suite.
🔬 Code Review — Deep Analysis
Code Review — PR #37842
Reviewed artifact: raw submitted PR fix materialized at 186863b5b8a6f1226517792939f656bc1239a6f4 (local squash of PR head 43b67e69d30804df8d82648dc5ca0db791cbc9fe), against PR base net11.0. No try-fix or pr-plus-reviewer candidate was read or reviewed. No fixes applied, no tests run, no GitHub comments posted, no approve/request-changes.
Diff surface: 1 file, +7/-0 — src/Core/src/Handlers/View/ViewHandler.cs (MapToolTip).
if (view is IToolTipElement tooltipContainer)
{
if (handler.IsConnectingHandler() && tooltipContainer.ToolTip is null)
{
return;
}
handler.ToPlatform().UpdateToolTip(tooltipContainer.ToolTip);
}Independent Assessment
(Formed from code alone, before reading the PR description or any prior-review surface.)
What this changes: MapToolTip (registered on the shared ViewHandler.ViewMapper under nameof(IToolTipElement.ToolTip), so it runs for every IView on every platform) now returns early — without touching the platform view — when both conditions hold: the handler is in the Connecting state, and the virtual view's ToolTip is null. Every other combination (Connecting + non-null, Reconnecting + any value, Connected + any value, i.e. all dynamic runtime updates) reaches handler.ToPlatform().UpdateToolTip(...) exactly as before.
Inferred motivation: Eliminate a per-view no-op platform call during initial handler connection — the overwhelmingly common case, since ToolTip is null for essentially all views. The avoided work is platform-specific and real, though small per view:
- iOS/MacCatalyst (
src/Core/src/Platform/iOS/ViewExtensions.cs:679):UpdateToolTipcallsGetToolTipInteraction()(:649), which for any non-UIControlenumeratesplatformView.Interactions— an ObjC-marshalled array walk per view — then does nothing becausetextis null/empty and no interaction exists. - Android (
src/Core/src/Platform/Android/ViewExtensions.cs:484): a JNI-crossingTooltipCompat.SetTooltipText(view, null)per view. - Windows (
src/Core/src/Platform/Windows/ViewExtensions.cs:282): aToolTipService.SetToolTip(platformView, null)DependencyProperty write per view.
Is the approach sound? Yes, and it is not a novel pattern — it is the file's established connect-time-skip convention, applied to one more mapper. Sixteen-plus in-tree precedents use the identical shape, most notably the structurally identical MapContextFlyout at src/Core/src/Handlers/View/ViewHandler.Windows.cs:161 (if (handler.IsConnectingHandler() && contextFlyoutContainer.ContextFlyout is null) return;), plus MapAutomationId (:455), MapClip (:468), MapShadow (:487), MapOpacity (:434), MapFlowDirection (:411), and the Min/Max width/height family (:265–:319). The in-file comment at ViewHandler.cs:39 documents the convention explicitly. A simpler alternative (removing the mapper entry, or gating on a platform-side null check) would be worse: the former breaks dynamic updates, the latter still pays the platform call.
Correctness of the state predicate — the load-bearing detail. ElementHandlerState (src/Core/src/Handlers/ElementHandlerState.cs) defines MappingProperties = 0x1, Connecting = MappingProperties | 0x2, Reconnecting = MappingProperties | 0x4. IsConnectingHandler() (src/Core/src/Handlers/InternalElementHandlerExtensions.cs:21) is State.HasFlag(Connecting), which requires both 0x1 and 0x2. Reconnecting (0x1|0x4) therefore does not satisfy it. In ElementHandler.SetVirtualView (src/Core/src/Handlers/Element/ElementHandler.cs:55-63), Connecting is assigned only on the PlatformView is null branch — immediately before CreatePlatformElement() — while an existing platform view being bound to a new virtual view takes the else branch and gets Reconnecting. The guard consequently cannot suppress the stale-state-clearing update on the reuse path, which is exactly the path that needs it.
Reconciliation with PR Narrative
Author claims: (1) Skips the platform ToolTip update while a new handler is connecting when ToolTip is the default null; (2) reconnects still execute the update so a reused platform view can clear stale state; (3) dynamic non-null and null updates are unchanged; (4) benchmark shows the effect is below simulator noise and no aggregate speedup is claimed; (5) three independent model reviews found the guard safe and consistent with existing connect-time guards.
Agreement/disagreement: Claims (1)–(3) are verified against source and match my independent trace exactly, including the non-obvious part — claim (2) is true because of the HasFlag bit layout and the PlatformView is null branch in SetVirtualView, not merely by assertion. Claim (4) is appropriately modest and matches what the code can deliver; the change removes one no-op platform call per view per connect, which is a micro-optimization, and the author explicitly does not claim an aggregate win — I have no basis to dispute a benchmark whose harness is not in the diff, and nothing in the diff depends on it being right. Claim (5) is narrative about process, not evidence; I did not rely on it and verified the same axes independently. No disagreement found.
One nuance the narrative does not spell out, which I record for completeness rather than as a defect (see 💡 below): "connecting" means "the handler just created its platform view via CreatePlatformElement()", so a custom handler whose CreatePlatformView returns a pre-configured or pooled native view carrying a native tooltip will now retain it instead of having it cleared. That is a native-default-preservation improvement consistent with the ContextFlyout precedent, not a regression.
Prior Review Reconciliation
All three surfaces queried anonymously (gh is unauthenticated in this environment; anonymous REST used per the skill's fallback), bodies read in full, untruncated:
| Prior ❌ Error Finding | Source | Status | Evidence |
|---|---|---|---|
| (none) | Surface 1 — top-level review bodies | n/a | Single review: copilot-pull-request-reviewer[bot], state COMMENTED. Body is a neutral "Pull request overview" restating the short-circuit and the preserved reconnect/dynamic behavior. No ❌, no ⚠️, no severity tags, no requested change. |
| (none) | Surface 2 — inline review comments | n/a | /pulls/37842/comments returned an empty set. |
| (none) | Surface 3 — PR issue comments | n/a | Only github-actions[bot] dogfood instructions, two azure-pipelines[bot] run acknowledgements, and two kubaflo slash-commands (/review -b improved-reviewer -p ios, /azp run). No findings. |
No prior ❌ Error findings found. Rule #5 override does not apply.
Blast Radius Assessment
(Required — this modifies a shared Core handler mapper on the property-mapping path for every view.)
- Runs for all instances: yes.
MapToolTipis aViewMapperentry, so it executes for everyIViewon Android/iOS/MacCatalyst/Windows/Tizen during_mapper.UpdateProperties(this, VirtualView)(ElementHandler.cs:96) and on every subsequentUpdateValue(nameof(ToolTip)). Breadth is maximal — which is precisely why the guard's narrowness matters: it fires only in theConnecting ∧ ToolTip is nullintersection, where all three platform implementations are provably no-ops on a freshly created platform view. - Startup impact: yes, but strictly subtractive. The guard runs during initial handler connection, i.e. page/app startup. It adds one enum flag test and one null test and removes work; it introduces no allocation, no new state, no new dependency, and no new failure surface. It cannot itself throw:
IsConnectingHandler()is null-safe viaas ... ?? false, and theis IToolTipElementpattern already excludes null. - Static/shared state: no. No new fields, no caching, no latch that persists beyond the call. The predicate is read fresh from
_handlerState, whichElementHandlersets deterministically on eachSetVirtualViewand finalizes toConnectedatElementHandler.cs:98. - Frequently-regressed families: not a member of any (CollectionView, CarouselView, Image/Graphics, Theme/Style, Gesture/Tap, Button/Entry, Toolbar, Shell/TabBar). It does, however, execute inside all of them, so I ran the reuse/recycle probes below explicitly rather than relying on family classification.
- Public API surface: unchanged.
MapToolTip's signature, visibility, and mapper registration are untouched; noPublicAPI.*.txtchange is required or present.
CI Status
(Per Step 5. gh pr checks 37842 --repo dotnet/maui --required could not be used — gh is unauthenticated here and returned To get started with GitHub CLI, please run: gh auth login. This is the tool-unavailable case, so I recorded the gap and pivoted to anonymous read-only retrieval of check-runs for head SHA 43b67e69, per the skill's Rule #7 / anonymous-fallback guidance. Required-vs-optional annotation is not available anonymously, so the classification below is conservative.)
- Required-check result: undetermined (mixed — one failure, many pending). Observed on head SHA:
maui-pr (Build .NET MAUI Build macOS (Debug))→ failure;maui-pr (Pack .NET MAUI Pack macOS)andmaui-pr-devicetests (... Build Device Tests (CoreCLR))→ success;Build Analysis,Build Windows (Debug),Build macOS (Release),Pack Windows, allmaui-pr-devicetestsdevice legs (Android/iOS/MacCatalyst/Windows/Mono), and the fullmaui-pr-uitestsmatrix → in_progress; the Helix unit-test and integration-test legs → cancelled (consistent with the superseding/azp runre-queue bykubafloat 12:51Z). - Classification: undetermined. I did not run, re-run, or diagnose any build, and I did not invoke
azdo-build-investigator(out of scope for this pass). I therefore cannot attribute the macOS Debug failure to the PR or to infrastructure. Noting only what is checkable statically: the diff is seven lines inside an existing#if PLATFORMblock in a file that already compilesIsConnectingHandlerusages under the same conditions elsewhere (ViewHandler.cs:259and following), uses no new symbol orusing, and changes no signature — so a PR-caused compile break in this diff is not apparent from the source. That is an observation, not a clearance; the failure remains unclassified. - Test coverage: none added, and none exercised. The Gate was supplied as SKIPPED — no tests detected; per instruction I did not rerun gate verification and did not add tests. For calibration: no in-tree test asserts the connect-time skip for any of the sibling guards (a repo-wide search for
IsConnectingHandler/ContextFlyout is nullundersrc/Core/testsandsrc/Controls/testsreturns nothing), so the absence of a test here is consistent with the convention this PR follows rather than a deviation from it.src/Core/tests/DeviceTests/Handlers/View/ViewHandlerTests.csdoes carry tooltip coverage that exercises the unchanged apply path. - Action taken: recorded the
ghauth gap explicitly; used anonymous check-runs; capped confidence at low; withheldLGTMper Rule #6 (red-or-pending CI). No investigator skill invoked, no second reviewer invoked.
Findings
No ❌ Error findings. No
inline-findings.json is written as an empty JSON array [] — there are no actionable raw file:line findings on the added/modified RIGHT-side lines. Every axis I probed (connect/reconnect semantics, mapper customization, all platform paths, default/null and dynamic transitions, reused platform views, custom handlers and custom platform views, lifecycle set/clear paths) resolved to correct behavior, and I could not construct a concrete failure scenario. Per the skill's "avoid false positives" and "don't pile on" constraints, I am not manufacturing a comment to fill the surface. The single 💡 below is a non-blocking observation that requests no code change, so it is intentionally not posted inline.
💡 Suggestion — Optional one-line comment documenting why the reconnect path is deliberately excluded (no code change requested)
src/Core/src/Handlers/View/ViewHandler.cs:658 — the guard's safety rests on a bit-layout detail that is invisible at the call site: Reconnecting is MappingProperties | 0x4 while IsConnectingHandler() tests HasFlag(Connecting) = MappingProperties | 0x2, so a reused platform view still receives UpdateToolTip(null) and clears its stale tooltip. A future refactor of ElementHandlerState (for example, making Reconnecting imply Connecting) would silently convert this guard into a stale-tooltip bug on the recycle path. A brief comment such as // Reconnect is deliberately not skipped: a reused platform view must clear stale tooltip state. would pin that invariant. Purely optional — the existing sibling guards (MapContextFlyout at ViewHandler.Windows.cs:161, MapClip at :468, MapShadow at :487) carry no such comment either, so adding one here is a nicety, not a consistency requirement, and its absence is not a defect. If the orchestrator applies at most one consolidated candidate patch, the surgical form is: insert that single comment line immediately above line 658. Nothing else in this file warrants modification.
Failure-Mode Probing
(Deliberately adversarial; each probe traced to exact state transitions rather than assumed.)
- Early-return guard above downstream side effects — enumerate every path that sets and clears the latch, then trace a repeat call (Principle #6). This is the highest-risk shape in the diff, so it gets the full trace. The "latch" is not a stored flag but the
_handlerStatefield, and it has exactly three writers, all inElementHandler.SetVirtualView/its completion:ConnectingatElementHandler.cs:57(only whenPlatformView is null, immediately beforeCreatePlatformElement()),Reconnectingat:62(theelse— platform view already exists), andConnectedat:98(unconditionally, after_mapper.UpdateProperties). There is no path that re-entersConnectingfor an already-created platform view, and no path that leaves the state stuck inConnectingafter mapping completes. Repeat-call trace: any post-connectUpdateValue(nameof(ToolTip))(:104) runs with stateConnected, soHasFlag(Connecting)is false and the guard is fully transparent. The guard is therefore not a persistent latch at all — it is a one-shot predicate that is provably false for every call after the initial mapping pass. Disproven as a defect. - Reused platform view whose previous virtual view had a tooltip, rebound to a virtual view with
ToolTip == null(CollectionView/CarouselView cell recycling,TemplatedCellrebinding, Shell tab switching).SetVirtualViewfindsPlatformViewnon-null →_handlerState = Reconnecting(:62) →HasFlag(Connecting)is false → guard does not fire →UpdateToolTip(null)runs → AndroidTooltipCompat.SetTooltipText(view, null)clears; WindowsToolTipService.SetToolTip(view, null)clears; iOSGetToolTipInteraction()finds the existing interaction and setsDefaultToolTip = null(iOS/ViewExtensions.cs:696-698). Stale tooltips are cleared on exactly the path that can produce them. This is the scenario most likely to have been broken, and it is not. - Handler disconnect followed by re-add (Shell tab switch, view removed and re-added).
DisconnectHandlerdoes not nullPlatformView, so a subsequentSetVirtualViewtakes theelsebranch →Reconnecting→ tooltip update runs. No skip, no stale state. Consistent with the maui-expert-reviewer caution against eagerly nulling state inDisconnectHandler. - Custom handler whose
CreatePlatformViewreturns a pooled or pre-configured native view already carrying a tooltip. State isConnecting(PlatformView is nullbefore creation),ToolTipis null → guard fires → the native tooltip is preserved rather than cleared. This is the one genuine behavioral delta versus base. It is (a) identical to the accepted behavior of theContextFlyoutprecedent atViewHandler.Windows.cs:161and theClip/Shadow/AutomationIdguards, and (b) the desired direction under the Native Platform Defaults Preservation dimension — a MAUInulldefault should not stomp a value the platform view legitimately carries. Requires an exotic custom handler to observe, and observes an improvement. Not a defect; recorded for transparency. - Does
AppendToMapping/PrependToMapping/ a replaced mapper entry silently lose user code? No. The guard is inside the defaultMapToolTipbody, not in the mapper dispatch.PropertyMapperstill invokes the chain; prepended code runs before, appended code runs after, and a wholesale replacement never calls this method at all. A user who callshandler.UpdateValue(nameof(IToolTipElement.ToolTip))from inside their ownConnectHandler(state stillConnecting) with a null tooltip gets a no-op — but on a freshly created platform view that no-op is behaviorally identical to base. This matches the documented convention atViewHandler.cs:39("the single mappers will behave as noop thanks to thehandler.IsConnectingHandler()check. The end user can still replace the mappers or append code to them, and it will be executed."). - Dynamic transitions:
null → "text"and"text" → nullat runtime. Both occur with stateConnected(set at:98before any user interaction). Guard is inert;UpdateToolTipruns with the new value. The"text" → nullcase — the one where an incorrectly-scoped guard would leave a ghost tooltip visible — is unaffected because the state test fails, independently of the null test. Note the guard's&&ordering is also the cheap-check-first form the perf dimension asks for, and both operands are side-effect-free, so short-circuit order carries no semantic risk. - Null
PlatformView, nullMauiContext, nullHandler, nullParent. The guard executes strictly beforehandler.ToPlatform(), so on the skip path it removes a dereference rather than adding one. On the non-skip pathToPlatform()is reached under exactly the same conditions as base — no null-safety regression, no new throw site.IsConnectingHandler()degrades safely tofalsefor any handler not implementingIElementHandlerStateExhibitor(as ... ?? false), which is the conservative direction: an unknown handler type gets the old unconditional behavior. - Multiple subscriptions / leak accumulation across handler lifecycle. Not applicable in a new way: the change adds no subscription, no interaction, and no reference. On iOS it can only reduce
UIToolTipInteractionchurn, never increase it — and on the skip path it avoids adding anything toplatformView.Interactions. - Tizen.
src/Core/src/Platform/Tizen/ViewExtensions.csalso definesUpdateToolTipand is inside the same#if PLATFORMblock. The guard is platform-agnostic and the Tizen path likewise only skips a null-value write on a fresh view. No platform is left behind or specially cased — the maui-expert-reviewer's "don't fix one platform only" check is satisfied by construction, since the change is in shared code. - Could the guard mask a real ordering dependency — e.g. is
MapToolTiprelied upon during connect to initialize something other than the tooltip? No. All four platform implementations do nothing but write tooltip text/interaction state; none allocates a container, registers a listener, or has a side effect another mapper depends on.ToPlatform()itself is a pure accessor (ContainerView ?? PlatformView) and is not being invoked for its side effects. There is no hidden initialization being skipped.
External Output Contract
Not applicable. The change classifies no external tool output — it introduces no regex, no string literal matched against console/CI logs, CLI stdout, exit-code lines, or file-format text. Its only predicates are an internal enum flag test (IsConnectingHandler()) and a null test on a managed property.
Trim/AOT Evidence Chain
Not applicable. The change touches no RequiresUnreferencedCode, RequiresDynamicCode, DynamicallyAccessedMembers, FeatureGuard, FeatureSwitchDefinition, or IL2026/IL3050 suppression, and adds no reflection, Type.GetType, or Activator.CreateInstance. The Trimming/AOT conditional dimension is not triggered.
Verdict: NEEDS_DISCUSSION
Confidence: low
Justification against the calibration tables: blast radius is shared Core infrastructure on the startup/property-mapping path → max low by the Blast Radius table, independent of anything else. Evidence caps compound it: CI is red-or-pending and unclassified (macOS Debug failure plus a large in-progress matrix on head SHA, with gh unauthenticated so required-check annotation was unavailable) → max low, and no relevant tests ran (Gate supplied as SKIPPED — no tests detected) → max low. Per Rule #6, LGTM is prohibited while required checks are failing, pending, or undetermined, which is what drives NEEDS_DISCUSSION here rather than any code defect. Confidence is confidence in the safety recommendation, not in the code analysis: I am considerably more certain that this seven-line guard is behaviorally correct than the low rating suggests — the rating reflects unverified CI and zero executed test coverage, not an un-disproven failure mode. There are no un-disproven failure modes.
Summary: The change applies ViewHandler.cs's established connect-time-skip convention to MapToolTip, skipping a provable no-op platform call on every view during initial connect while leaving reconnect, dynamic, and clearing paths fully intact — the reconnect path is preserved by the ElementHandlerState bit layout (Reconnecting does not satisfy HasFlag(Connecting)) combined with SetVirtualView assigning Connecting only when it is about to create a new platform view, so a recycled platform view still gets its stale tooltip cleared. It is structurally identical to the in-tree MapContextFlyout precedent, adds no state, no allocation, and no new throw site, and is verified correct on Android, iOS/MacCatalyst, Windows, and Tizen. No prior reviewer flagged any ❌ finding, and I found none, so inline-findings.json is []. The verdict is NEEDS_DISCUSSION solely because required CI has an unclassified macOS Debug failure with the remainder still in progress and no tests were executed — human sign-off should follow a green CI run rather than any change to the code.
🛠️ Try-Fix — Analysis & Comparison
PR #37842 Try-Fix Aggregate
Candidate 1 — Platform-View ToolTip Application-State Idempotence
- Model:
claude-opus-5 - Result:
Blocked - Candidate narrative:
CustomAgentLogsTmp/PRState/37842/PRAgent/try-fix-1/content.md - Artifacts:
CustomAgentLogsTmp/PRState/37842/PRAgent/try-fix/attempt-1 - Files changed / diff: None; the captured diff is empty.
- Test:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Core -Platform ios -TestFilter "Category=View"was not run because the candidate could not be applied. - Self-review: 0 findings;
reviewer-findings.jsoncontains[]. - Restore: The exact restore script ran and reported
No baseline state found/Restored False, which is expected because baseline setup failed before creating state or editing source.
Approach
Track on the ViewHandler instance which platform view last received a non-null tooltip. A null tooltip would skip UpdateToolTip only when the tracked reference is not the current platform view. This would avoid native work for initial and later redundant null maps without consulting IsConnectingHandler(), while allowing null to clear a tooltip previously applied to the current platform view.
Difference from the PR
The PR uses lifecycle phase plus default value (IsConnectingHandler() and null) to identify redundant first-connect work. Candidate 1 instead proposed application-state idempotence: the handler would track whether the current platform view could have tooltip state to clear. That mechanism could also suppress redundant post-connect null maps while preserving first-connect non-null application, dynamic updates, and reconnect clearing.
Blocker Analysis
The mandatory EstablishBrokenBaseline.ps1 invocation rejected the pre-existing dirty worktree. The reported changes are harness-owned files under .github/ and eng/; the source tree itself is clean. The script did not create .github/.baseline-state.json, so no RevertedFiles allow-list existed and applying the design would have violated the try-fix restoration boundary. Forbidden cleanup commands and modification of harness-owned changes were not used. The candidate therefore stopped before implementation and testing.
Candidate 2 — iOS Native ToolTip-Interaction Presence Probe
- Model:
gpt-5.6-sol - Result:
Blocked - Candidate narrative:
CustomAgentLogsTmp/PRState/37842/PRAgent/try-fix-2/content.md - Artifacts:
CustomAgentLogsTmp/PRState/37842/PRAgent/try-fix/attempt-2 - Files changed / diff: None; the captured diff is empty.
- Test:
pwsh .github/skills/run-device-tests/scripts/Run-DeviceTests.ps1 -Project Core -Platform ios -TestFilter "Category=View"was not run because the candidate could not be applied. - Self-review: 0 findings;
reviewer-findings.jsoncontains[]. - Restore: The exact restore script ran and reported
No baseline state found/Restored False, which is expected because baseline setup failed before creating state or editing source.
Approach
Use the existing iOS/MacCatalyst UIView.GetToolTipInteraction() query as the source of truth before a null update. If the virtual tooltip is null and the current native view has no UIToolTipInteraction, skip UpdateToolTip; if an interaction exists, retain the null update so it clears native state. Non-null values would always update.
Difference from Earlier Approaches
Candidate 2 uses neither the PR's connection-phase inference nor candidate 1's handler field/cache. It observes current native state at call time: a new iOS view without a tooltip interaction needs no clear, while a dynamically cleared or reconnected view with an interaction still does. This is a platform-state probe rather than lifecycle classification or remembered application history.
Blocker Analysis
The mandatory baseline command again rejected the same pre-existing harness-owned .github/ and eng/ changes and did not create .github/.baseline-state.json. With no RevertedFiles allow-list, the candidate could not safely edit the target file or run a meaningful validation. It did not bypass cleanup safeguards or alter the first candidate.
Aggregate Outcome
Two candidates were generated, one per required model, and both completed as Blocked before implementation. No candidate source diff exists and no test result can support a Pass or Fail verdict. Both attempts preserved the materialized PR and pre-existing harness changes, produced the required attempt artifacts, and ran the exact restore command.
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr-plus-reviewer
The submitted guard is behaviorally sound: it skips only the default-null ToolTip mapping on first connection, while reconnects and dynamic null/non-null updates still reach the platform implementation. The expert review found no error or warning and produced no inline findings. pr-plus-reviewer wins narrowly because it preserves that implementation, documents the otherwise non-obvious reconnect invariant, and passed the required focused iOS validation.
Candidate Comparison
| Rank | Candidate | Implementation | Validation | Assessment |
|---|---|---|---|---|
| 1 | pr-plus-reviewer |
Raw PR fix plus one invariant comment; no runtime change | Pass: 68 passed, 0 failed, 1 ignored | Best combination of the sound minimal fix, explicit lifecycle intent, and focused regression evidence |
| 2 | pr |
Minimal connect-time null guard using the established IsConnectingHandler() pattern |
Gate skipped; no PR tests detected; expert CI snapshot was red/pending and unclassified | Correct and simpler than either alternative, but lacks the winning candidate's clarification and completed targeted run |
| 3 | try-fix-2 |
Design only: probe iOS native UIToolTipInteraction state before a null update |
Blocked before implementation; no tests run | Direct native-state reasoning is attractive, but it is platform-specific and still performs the iOS interaction lookup that motivates the optimization |
| 4 | try-fix-1 |
Design only: cache the platform view that last received a non-null ToolTip | Blocked before implementation; no tests run | Broader redundant-null suppression, but adds mutable handler state and lifecycle/cache invalidation risk for a micro-optimization |
Comparative Reasoning
pr
The raw PR changes only MapToolTip. Connecting is assigned only when a platform view is about to be created; an existing platform view enters Reconnecting, which does not satisfy IsConnectingHandler(). Therefore a reused view still receives UpdateToolTip(null) and clears stale state. Mapper replacement/append behavior, first-connect non-null mapping, and connected-state updates are unchanged. The expert reviewer found no concrete failure mode.
pr-plus-reviewer
The candidate adds only a comment explaining why reconnect must not be folded into the early return. git diff --check passed, and the exact targeted iOS Category=View command passed from the required sandbox. Build and test artifacts were rooted in that sandbox, so the result is valid candidate evidence. This candidate is functionally as safe and minimal as the PR while making the fragile lifecycle assumption discoverable to future maintainers.
try-fix-1
This candidate never reached an applied diff because baseline setup was blocked by the raw worktree's harness-owned changes. Its proposed per-handler cache could skip more redundant null mappings, but it introduces persistent state that must remain synchronized with platform-view replacement, external/native tooltip changes, custom handlers, and lifecycle transitions. That complexity is not justified by the measured micro-optimization and has no regression evidence.
try-fix-2
This candidate also remained design-only and unvalidated. Inspecting native iOS state can distinguish a real clear from a no-op, but it narrows a shared optimization to iOS and invokes the same interaction lookup that UpdateToolTip(null) already performs. It offers no demonstrated advantage over the shared lifecycle guard.
Review and Evidence Constraints
- Expert verdict:
NEEDS_DISCUSSION, low confidence solely because the review-time CI snapshot was failing/pending and the supplied Gate was skipped; no code defect or unresolved failure mode was found. - Gate: skipped because the PR adds no tests. It was not rerun.
- The focused candidate validation passed, but it is existing category coverage rather than a new assertion that the first-connect native call is skipped.
- Both try-fix candidates are ranked below implemented candidates because they were blocked before producing a diff or regression result.
Because pr-plus-reviewer is the winner rather than the submitted raw pr, this report must request changes. The required change is the one-line lifecycle-invariant comment captured in pr-plus-reviewer/reviewer.patch; no functional rewrite is recommended.
📱 UI Tests — ViewBaseTests
Detected UI test categories: ViewBaseTests
✅ Deep UI tests — 112 passed, 0 failed across 1 category on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
ViewBaseTests |
112/112 ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🧭 Next Steps — reviewer changes required
The reviewer-enhanced candidate identified changes that are not yet in the submitted PR.
Why: The reviewed PR implementation is correct and minimal; pr-plus-reviewer preserves it while documenting the non-obvious reconnect-clearing invariant. It also passed the required focused iOS validation with 68 passed, 0 failed, and 1 ignored, while both try-fix alternatives were blocked before implementation.
Address the actionable findings in this review before merging.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description
Skips the platform ToolTip update while a new handler is connecting when the virtual view has the default
nullToolTip.Reconnects still execute the update so a reused platform view can clear stale state. Dynamic non-null and null updates are unchanged.
Benchmark
The local Sandbox harness is not included in this PR. Release iOS simulator, iPhone 16 Pro / iOS 18.4:
ToPlatform(55 elements)ToPlatform(14 elements)The mapper activity count remains 480 because attribution wraps the mapper entry even when the new guard returns immediately. This micro-optimization is below end-to-end simulator noise and no aggregate speedup is claimed. The run completed all 70 benchmark results with
run-end|ok.Review
Independent GPT, Claude, and Gemini reviews traced connection, reconnect, null/default values, dynamic updates, custom implementations, and all platform paths. All three found the guard safe and consistent with existing connect-time mapper guards.