Skip to content

Commit 48b0515

Browse files
spec(047): address PR #440 CR feedback (Copilot reviewer)
* Carve GridViewDescriptor — its ItemsHost<> strategy pre-mounts every item into GridView.Items (no virtualization), diverging from legacy MountGridView's ItemsSource+CCC lazy realization. A|B tests still pass, but production memory/lifecycle would silently regress; closing needs hand-coded GridViewHandler or RecyclingItemsHost<> shape. Updated coverage table: 75 routed / 12 deferred / 8 primitives. * Align legacy UpdateNavigationView with V1 NavigationViewDescriptor — add PaneDisplayMode, IsSettingsVisible, conditional PaneTitle, MenuItems rebuild on ref-change, and SelectedItem re-selection (with new FindNavItemByTag helper). Previously legacy only wrote a subset of these on update; reviewer correctly flagged the A|B divergence for record-with updates that change MenuItems / Selected. * Fix PreMountedItems<> release fallback at ChildrenStrategy.cs:577 — the count-drift path was clamping oldCount=items.Count and continuing positional reconciliation, which would index past oldSource bounds when items.Count > oldSource.ItemCount or skip stale source items' teardown when smaller. Replaced with the same full-rebuild path as the type-mismatch release fallback above. * Doc fix: remove ModifiedElement from the tracker's composition- primitive carve list — it's unwrapped before V1/legacy dispatch at the top of Reconciler.Mount, not a switch arm; coverage table stays at 8 primitives. Validation: dotnet test 9134/0; full V1 ON selftest 4410/0; full V1 OFF selftest 4410/0 (each suite required one re-run to clear the known FloatRoot/Reliability/DockHooks docking flake family). Ignored 9 github-code-quality bot comments: 8 are integer-valued double == checks in test fixtures (values are assigned constants, exact equality is reliable); 1 claims WinUI FontFamily is IDisposable (it is not). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2f70c04 commit 48b0515

5 files changed

Lines changed: 134 additions & 19 deletions

File tree

docs/specs/047-extensible-control-model.md

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1439,14 +1439,14 @@ ARM64 stable-AC re-capture on `LAPTOP-4MEP83VI` remains deferred for the §14 ra
14391439

14401440
| Bucket | Arms | Notes |
14411441
|---|---:|---|
1442-
| Routed through V1 (Phase 1 handler or Phase 3 descriptor) | 76 | Production dispatch path when V1 ON |
1443-
| Reachable-but-deferred (overlays 7 + NavigationHost 1 + TabView 1 + XamlHost/Page 2) | 11 | Follow-up PR closes these |
1442+
| Routed through V1 (Phase 1 handler or Phase 3 descriptor) | 75 | Production dispatch path when V1 ON |
1443+
| Reachable-but-deferred (overlays 7 + NavigationHost 1 + TabView 1 + GridView 1 + XamlHost/Page 2) | 12 | Follow-up PR closes these |
14441444
| Intentionally above V1 (Reactor composition primitives) | 8 | Permanent carve; Phase 4 keeps legacy arms |
14451445
| **Total switch arms** | **95** ||
14461446

1447-
- **Coverage of V1-reachable surface:** 76 / 87 ≈ **87%** (excludes the 8 composition primitives that are permanently above the protocol).
1448-
- **Coverage of all element-type switch arms:** 76 / 95 ≈ **80%**.
1449-
- **Path to 100% reachable:** the next PR ports the 11 deferred (overlays, NavigationHost, TabView gap closure, XamlHost/Page unification). Phase 4 cleanup follows.
1447+
- **Coverage of V1-reachable surface:** 75 / 87 ≈ **86%** (excludes the 8 composition primitives that are permanently above the protocol).
1448+
- **Coverage of all element-type switch arms:** 75 / 95 ≈ **79%**.
1449+
- **Path to 100% reachable:** the next PR ports the 12 deferred (overlays, NavigationHost, TabView gap closure, GridView CCC virtualization, XamlHost/Page unification). Phase 4 cleanup follows.
14501450

14511451
**Phase 3 finish advisory perf** — Cloud PC x64 re-capture under `docs/specs/047/phase3-results/CPC-ander-YTZ3O-x64-advisory/2026-05-28-phase3-finish-3x5/` (n=15, 3 launches × 5 reps). V1 ON (descriptors) vs V1 OFF (today), against prior `2026-05-27-phase3-closeout-3x5/`:
14521452

docs/specs/tasks/047-extensible-control-model-implementation.md

Lines changed: 39 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -964,10 +964,19 @@ the audit at the end of `spec/047-phase3-finish`:
964964
up-front into `FlipView.Items` (no `ContainerContentChanging` to
965965
drive realization) and positionally reconciles via
966966
`Reconciler.ReconcileV1Child` on Update.
967-
- **Untyped items hosts (CLOSED — Phase 3 completion):**
968-
`GridViewElement` (plain Element[]), `ItemsViewElementBase`,
969-
`ItemContainerElement` — all ported as standard descriptors and
970-
registered in `RegisterV1BuiltInHandlers`.
967+
- **Untyped items hosts (CLOSED — Phase 3 completion, partial):**
968+
`ItemsViewElementBase`, `ItemContainerElement` — ported as
969+
standard descriptors and registered in `RegisterV1BuiltInHandlers`.
970+
`GridViewElement` (plain Element[]) — descriptor authored
971+
(`GridViewDescriptor`) but **carved during PR #440 CR**: the
972+
descriptor's `ItemsHost<>` strategy pre-mounts every item into
973+
`GridView.Items` (one container per item, no virtualization),
974+
while the legacy `MountGridView` arm uses
975+
`ItemsSource = Range(0..N) + ItemTemplate + ContainerContentChanging`
976+
to realize containers lazily (matches Phase 1 `ListViewHandler`).
977+
Closing this needs either a hand-coded `GridViewHandler` or a new
978+
ChildrenStrategy variant wrapping CCC. Tracked alongside TabView /
979+
overlays / NavigationHost gap-closure.
971980
- **Heavy / specialized controls (CLOSED — Phase 3 completion):**
972981
`WebView2Element`, `NavigationViewElement`, `TitleBarElement`,
973982
`MediaPlayerElementElement`, `AnimatedVisualPlayerElement`,
@@ -1010,6 +1019,19 @@ scoped carve list documented inline in `RegisterV1BuiltInHandlers`):**
10101019
Closing them requires engine work (post-children mount-hook so
10111020
`SelectionChanged` subscribes after children-add + an
10121021
`ImperativeBridged` shape for the named tab strip slots).
1022+
- **`GridViewDescriptor` (descriptor exists, registration carved
1023+
during PR #440 CR):** The descriptor's `ItemsHost<>` ChildrenStrategy
1024+
pre-mounts every item into `GridView.Items` (one container per item,
1025+
no virtualization). The legacy `MountGridView` arm uses
1026+
`ItemsSource = Range(0..N) + ItemTemplate + ContainerContentChanging`
1027+
to realize containers lazily — matching Phase 1
1028+
`ListViewHandler`. A|B tests pass either way (no fixture stresses
1029+
GridView scale), but production memory/lifecycle would silently
1030+
regress. Closing this needs either a hand-coded `GridViewHandler`
1031+
mirroring `ListViewHandler`'s CCC virtualization, or a new
1032+
ChildrenStrategy variant (e.g. `RecyclingItemsHost<>`) that wraps
1033+
the `ItemsSource` + `ContainerContentChanging` realization
1034+
contract.
10131035
- **Interop bridges:** `XamlHostElement`, `XamlPageElement`. V1
10141036
descriptors exist (`XamlHostDescriptor`, `XamlPageDescriptor`)
10151037
but stay unregistered because `XamlInterop.Register(reconciler)`
@@ -1021,11 +1043,15 @@ scoped carve list documented inline in `RegisterV1BuiltInHandlers`):**
10211043
protocol — Phase 4 cleanup keeps their legacy arms):**
10221044

10231045
- `ComponentElement`, `FuncElement`, `MemoElement`,
1024-
`ErrorBoundaryElement`, `CommandHostElement`, `ModifiedElement`,
1046+
`ErrorBoundaryElement`, `CommandHostElement`,
10251047
`Validation.FormFieldElement` /
10261048
`ValidationVisualizerElement` / `ValidationRuleElement`. These
10271049
orchestrate child reconciliation rather than wrap a single WinUI
10281050
control, so the V1 handler protocol does not apply.
1051+
(`ModifiedElement` is intentionally NOT in this list — it's
1052+
unwrapped to its wrapped element BEFORE dispatch at the top of
1053+
`Reconciler.Mount`, so it never reaches the switch and does not
1054+
count as a carved arm.)
10291055

10301056
**Phase 3 completion status (PR #440 — landed-pending-merge):**
10311057
Every element type in the production codebase either (a) routes
@@ -1046,21 +1072,22 @@ follow-up PRs land.
10461072

10471073
| Bucket | Arms | % of total |
10481074
|---|---:|---:|
1049-
| Routed through V1 (76 = 5 Phase 1 + 6 base-derived + 64 standard descriptors + 1 decorator) | 76 | 80% |
1050-
| Reachable-but-deferred (overlays 7, NavigationHost 1, TabView 1, XamlHost/Page 2) | 11 | 11.6% |
1075+
| Routed through V1 (75 = 5 Phase 1 + 6 base-derived + 63 standard descriptors + 1 decorator) | 75 | 79% |
1076+
| Reachable-but-deferred (overlays 7, NavigationHost 1, TabView 1, GridView 1, XamlHost/Page 2) | 12 | 12.6% |
10511077
| Intentionally above V1 (composition primitives — permanent carve) | 8 | 8.4% |
10521078
| **Total `Reconciler.Mount.cs` switch arms** | **95** | **100%** |
10531079

1054-
- **Coverage of V1-reachable surface (excludes 8 composition primitives):** 76 / 87 ≈ **87%**.
1055-
- **Coverage of all switch arms:** 76 / 95 ≈ **80%**.
1056-
- **Path to 100% reachable:** the follow-up PR closes the 11 deferred:
1080+
- **Coverage of V1-reachable surface (excludes 8 composition primitives):** 75 / 87 ≈ **86%**.
1081+
- **Coverage of all switch arms:** 75 / 95 ≈ **79%**.
1082+
- **Path to 100% reachable:** the follow-up PR closes the 12 deferred:
10571083
1. Port the 7 overlay descriptors (ContentDialog, Flyout, Popup, MenuBar, MenuFlyout, CommandBar, CommandBarFlyout) — needs a decorator strategy variant for modal lifecycle beyond `IDecoratorElementHandler`.
10581084
2. Refactor `NavigationHostElement` cleanup path so V1 can own it (internal-expose `MountNavigationHost` / `UpdateNavigationHost`, duplicate cleanup logic in the V1 handler, remove the `UnmountRecursive` intercept).
10591085
3. Close `TabViewDescriptor` gaps (engine post-children mount-hook + `ImperativeBridged` for named slots + port `BuildTabHeader` / `BuildPinButton` / `TryUpdatePinHeaderInPlace` + drag pipeline trampolines + conditional `SelectedIndex` write + in-place `CanUpdate`).
1060-
4. Unify `XamlInterop.Register` with V1 auto-registration so `XamlHostElement` / `XamlPageElement` descriptors can register without `EnsureRegistrableElementType` clash.
1086+
4. Close `GridViewDescriptor` lifecycle gap — either author a Phase 1 hand-coded `GridViewHandler` mirroring `ListViewHandler`'s CCC virtualization, or introduce a `RecyclingItemsHost<>` ChildrenStrategy variant.
1087+
5. Unify `XamlInterop.Register` with V1 auto-registration so `XamlHostElement` / `XamlPageElement` descriptors can register without `EnsureRegistrableElementType` clash.
10611088

10621089
Phase 4 cleanup (deletion of legacy switch arms + `UseV1Protocol`
1063-
flag) is unblocked for the 76 routed arms today; the remaining 11
1090+
flag) is unblocked for the 75 routed arms today; the remaining 12
10641091
arms unblock as the follow-up PR lands each closure.
10651092

10661093
**Carry-forward known defects** (from Phase 1):

src/Reactor/Core/Reconciler.Update.cs

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1739,10 +1739,29 @@ void FinalizeOldPage(UIElement? oldCtrl, Element? oldElem, object? oldRoute)
17391739
private UIElement? UpdateNavigationView(NavigationViewElement o, NavigationViewElement n, WinUI.NavigationView nv, Action requestRerender)
17401740
{
17411741
nv.IsPaneOpen = n.IsPaneOpen; nv.IsBackEnabled = n.IsBackEnabled;
1742+
// Spec 047 §14 Phase 3 completion CR — match V1 NavigationViewDescriptor
1743+
// (PaneDisplayMode / IsSettingsVisible / PaneTitle / MenuItems / SelectedItem
1744+
// were previously update-only on the V1 path). Aligning the legacy arm so
1745+
// V1 ON ≡ V1 OFF for record-with updates that change any of these fields.
1746+
nv.PaneDisplayMode = n.PaneDisplayMode;
1747+
nv.IsSettingsVisible = n.IsSettingsVisible;
1748+
if (n.PaneTitle is not null && nv.PaneTitle != n.PaneTitle) nv.PaneTitle = n.PaneTitle;
17421749
if (!double.IsNaN(n.OpenPaneLength) && nv.OpenPaneLength != n.OpenPaneLength) nv.OpenPaneLength = n.OpenPaneLength;
17431750
if (!double.IsNaN(n.CompactModeThresholdWidth) && nv.CompactModeThresholdWidth != n.CompactModeThresholdWidth) nv.CompactModeThresholdWidth = n.CompactModeThresholdWidth;
17441751
if (!double.IsNaN(n.ExpandedModeThresholdWidth) && nv.ExpandedModeThresholdWidth != n.ExpandedModeThresholdWidth) nv.ExpandedModeThresholdWidth = n.ExpandedModeThresholdWidth;
17451752

1753+
if (!ReferenceEquals(o.MenuItems, n.MenuItems))
1754+
{
1755+
nv.MenuItems.Clear();
1756+
foreach (var item in n.MenuItems)
1757+
{
1758+
if (item.IsHeader)
1759+
nv.MenuItems.Add(new WinUI.NavigationViewItemHeader { Content = item.Content });
1760+
else
1761+
nv.MenuItems.Add(CreateNavItem(item));
1762+
}
1763+
}
1764+
17461765
// AutoSuggestBox / PaneFooter / PaneCustomContent reconcile in place
17471766
// when possible so the controls keep focus / scroll state across re-renders.
17481767
ReconcileChild(o.AutoSuggestBox, n.AutoSuggestBox,
@@ -1782,6 +1801,13 @@ void FinalizeOldPage(UIElement? oldCtrl, Element? oldElem, object? oldRoute)
17821801
}
17831802

17841803
SetElementTag(nv, n);
1804+
1805+
// Spec 047 §14 Phase 3 completion CR — match V1 NavigationViewDescriptor:
1806+
// re-select when SelectedTag drifts OR MenuItems ref changes (the rebuild
1807+
// above can re-create the previously-selected container).
1808+
if (o.SelectedTag != n.SelectedTag || !ReferenceEquals(o.MenuItems, n.MenuItems))
1809+
nv.SelectedItem = FindNavItemByTag(nv.MenuItems, n.SelectedTag);
1810+
17851811
if (o.OnSelectedTagChanged is null && n.OnSelectedTagChanged is not null)
17861812
nv.SelectionChanged += (s, args) =>
17871813
{
@@ -1794,6 +1820,21 @@ void FinalizeOldPage(UIElement? oldCtrl, Element? oldElem, object? oldRoute)
17941820
return null;
17951821
}
17961822

1823+
private static object? FindNavItemByTag(global::System.Collections.IEnumerable items, string? selectedTag)
1824+
{
1825+
if (selectedTag is null) return null;
1826+
foreach (var item in items)
1827+
{
1828+
if (item is WinUI.NavigationViewItem nvi)
1829+
{
1830+
if ((nvi.Tag as string) == selectedTag) return nvi;
1831+
var child = FindNavItemByTag(nvi.MenuItems, selectedTag);
1832+
if (child is not null) return child;
1833+
}
1834+
}
1835+
return null;
1836+
}
1837+
17971838
private UIElement? UpdateTitleBar(TitleBarElement o, TitleBarElement n, WinUI.TitleBar titleBar, Action requestRerender)
17981839
{
17991840
titleBar.Title = n.Title;

src/Reactor/Core/Reconciler.cs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,16 @@ internal Reconciler(ILogger? logger, bool? useV1Protocol, bool registerBuiltinHa
345345
/// (post-children mount-hook so SelectionChanged subscribes after
346346
/// children-add, ImperativeBridged for the named tab strip slots)
347347
/// tracked alongside the overlays + NavigationHost in the follow-up.</item>
348+
/// <item><b>Deferred — descriptor with lifecycle divergence from
349+
/// legacy</b> (follow-up PR) — <c>GridViewDescriptor</c>. The descriptor's
350+
/// <c>ItemsHost&lt;&gt;</c> strategy pre-mounts every item into
351+
/// <c>GridView.Items</c> (one container per item, no virtualization).
352+
/// The legacy <c>MountGridView</c> arm uses
353+
/// <c>ItemsSource = Range(0..N) + ItemTemplate + ContainerContentChanging</c>
354+
/// to realize containers lazily, matching the Phase 1
355+
/// <see cref="V1Protocol.Handlers.ListViewHandler"/> pattern. Closing
356+
/// this needs either a Phase 1 hand-coded <c>GridViewHandler</c> or a
357+
/// new ChildrenStrategy that wraps the CCC virtualization contract.</item>
348358
/// </list>
349359
/// </summary>
350360
private void RegisterV1BuiltInHandlers()
@@ -392,7 +402,23 @@ private void RegisterV1BuiltInHandlers()
392402
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.FlipViewDescriptor.Descriptor);
393403
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.FrameDescriptor.Descriptor);
394404
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.GridDescriptor.Descriptor);
395-
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.GridViewDescriptor.Descriptor);
405+
// Spec 047 §14 Phase 3 completion CR (Copilot review on PR #440) —
406+
// GridViewDescriptor stays carved. The descriptor's ItemsHost<>
407+
// strategy pre-mounts every item into GridView.Items (one container
408+
// per item, no virtualization, no recycle). The legacy MountGridView
409+
// path uses ItemsSource = Range(0..N) + ItemTemplate +
410+
// ContainerContentChanging to realize containers lazily (one per
411+
// visible viewport item) and unmount on RecycleQueue entry — same
412+
// pattern as the Phase 1 hand-coded ListViewHandler. Registering
413+
// GridViewDescriptor would not break A|B tests (no fixture stresses
414+
// GridView scale), but it would silently regress production memory
415+
// and lifecycle (item Mount/Unmount fires for every item up-front
416+
// instead of per-viewport, breaking the recycle contract). Closing
417+
// this needs either a Phase 1 hand-coded GridViewHandler mirroring
418+
// ListViewHandler, or a new ChildrenStrategy variant that wraps
419+
// ItemsSource + CCC. Tracked alongside the TabView/overlay/NavigationHost
420+
// gap-closure follow-up.
421+
// RegisterDescriptor(V1Protocol.Descriptor.Descriptors.GridViewDescriptor.Descriptor);
396422
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.HyperlinkButtonDescriptor.Descriptor);
397423
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.ImageDescriptor.Descriptor);
398424
RegisterDescriptor(V1Protocol.Descriptor.Descriptors.InfoBadgeDescriptor.Descriptor);

src/Reactor/Core/V1Protocol/ChildrenStrategy.cs

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -574,7 +574,28 @@ void IItemsBinderStrategy.Bind(FrameworkElement control, Element? oldElement, El
574574
items.Count == oldCount,
575575
"PreMountedItems<>: control items collection (" + items.Count
576576
+ ") drifted from previous source ItemCount (" + oldCount + ").");
577-
if (items.Count != oldCount) oldCount = items.Count;
577+
if (items.Count != oldCount)
578+
{
579+
// Release fallback for count drift — same shape as the
580+
// type-mismatch rebuild above. Cannot continue positional
581+
// reconciliation safely: if items.Count > oldSource.ItemCount
582+
// we'd index past oldSource bounds; if smaller, stale source
583+
// items would be skipped without element-aware teardown.
584+
for (int i = items.Count - 1; i >= 0; i--)
585+
{
586+
if (items[i] is UIElement orphan) reconciler.UnmountChild(orphan);
587+
items.RemoveAt(i);
588+
}
589+
for (int i = 0; i < newCount; i++)
590+
{
591+
var fresh = reconciler.Mount(newSource.BuildItemView(i), requestRerender);
592+
if (fresh is null)
593+
throw new global::System.InvalidOperationException(
594+
"PreMountedItems<>: item Element at index " + i + " mounted to a null UIElement.");
595+
items.Add(fresh);
596+
}
597+
return;
598+
}
578599

579600
int shared = global::System.Math.Min(oldCount, newCount);
580601
for (int i = 0; i < shared; i++)

0 commit comments

Comments
 (0)