Skip to content

fix(navigationview): reconcile menu items in place to preserve expansion#454

Merged
codemonkeychris merged 2 commits into
mainfrom
fix/navigationview-expand-collapse-rebuild-clobber
May 29, 2026
Merged

fix(navigationview): reconcile menu items in place to preserve expansion#454
codemonkeychris merged 2 commits into
mainfrom
fix/navigationview-expand-collapse-rebuild-clobber

Conversation

@codemonkeychris
Copy link
Copy Markdown
Collaborator

Problem

NavigationView with hierarchical menu items mis-behaved on every interaction:

  • clicking a category expanded it then immediately collapsed it (a flash);
  • clicking a child item collapsed its parent;
  • selection state churned on each render.

Root cause

UpdateNavigationView (and the V1 NavigationViewDescriptor) did
MenuItems.Clear() + full rebuild whenever the MenuItems array reference
changed. Consumers typically build a fresh MenuItems array each Render()
(e.g. via LINQ), so the ReferenceEquals guard never held and the rebuild
fired on essentially every re-render.

Per-item expansion lives only on the live NavigationViewItem — there is no
IsExpanded on NavigationViewItemData — so recreating the containers reset
expansion to false every time. Because selecting an item triggers a
re-render, the just-expanded category (or a child's parent) snapped shut on the
same click.

Fix

Reconcile menu items in place instead of clear-and-rebuild:

  • match existing NavigationViewItems by Tag;
  • update only changed Content / Icon;
  • recurse into children;
  • reuse the container so its runtime IsExpanded (and selection) survive the
    re-render.

A fast path skips all detaching when the item structure is unchanged (the
common case). Mirrored into the V1 NavigationViewDescriptor so V1 ON behaves
identically to V1 OFF. Mount-time selection now recurses so a child tag selects
correctly.

Tests

  • New E2E guard: NavigationView_Hierarchical host fixture +
    NavigationViewInteractionTests. Verified to fail on the pre-fix build
    (expanded category collapses after the selection re-render) and pass after.
  • Existing NavigationView self-tests, including the V1
    Desc_NavigationView_MountUpdate coverage, pass (0 failures).
  • Also adds a TreeView E2E guard exercising the Gallery's basic text-tree path.

Out of scope

Compact (icon-only) pane interaction is deferred to a follow-up.

NavigationView cleared and rebuilt every menu item whenever the MenuItems
array reference changed — which is effectively every render, since consumers
build a fresh array each Render(). Per-item expansion lives only on the live
NavigationViewItem (NavigationViewItemData has no IsExpanded), so each
re-render snapped expanded hierarchical categories shut, and selecting a child
collapsed its parent.

Reconcile menu items in place instead: match NavigationViewItems by Tag,
update only changed Content/Icon, recurse into children, and reuse the
container so IsExpanded survives the re-render. A fast path skips all
detaching when the structure is unchanged. Mirrored into the V1
NavigationViewDescriptor so V1 ON behaves identically to V1 OFF, and
mount-time selection now recurses so a child tag selects correctly.

Adds a NavigationView E2E guard (NavigationView_Hierarchical fixture +
NavigationViewInteractionTests) that fails on the pre-fix build and passes
after, plus a TreeView E2E guard exercising the Gallery's basic text-tree path.

Compact (icon-only) pane interaction is deferred to a follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread src/Reactor/Core/Reconciler.Update.cs Fixed
Comment thread src/Reactor/Core/Reconciler.Update.cs Fixed
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes hierarchical NavigationView updates so menu item containers are reconciled in place instead of being cleared and recreated, preserving runtime expansion and selection state across re-renders.

Changes:

  • Adds in-place NavigationView menu reconciliation in both legacy and V1 descriptor paths.
  • Updates mount-time selection to find nested items by tag.
  • Adds E2E fixtures/tests for hierarchical NavigationView and TreeView interaction regression coverage.
Show a summary per file
File Description
src/Reactor/Core/Reconciler.Update.cs Replaces clear-and-rebuild menu updates with Tag-based in-place reconciliation.
src/Reactor/Core/Reconciler.Mount.cs Recursively resolves initial SelectedTag for nested NavigationView items.
src/Reactor/Core/V1Protocol/Descriptor/Descriptors/NavigationViewDescriptor.cs Mirrors in-place menu reconciliation for V1 NavigationView behavior.
tests/Reactor.AppTests/Tests/NavigationViewInteractionTests.cs Adds E2E coverage for preserving hierarchical NavigationView expansion after selection re-renders.
tests/Reactor.AppTests/Tests/TreeViewInteractionTests.cs Adds TreeView interaction regression tests.
tests/Reactor.AppTests.Host/Fixtures/NavigationViewE2EFixtures.cs Adds the hierarchical NavigationView host fixture.
tests/Reactor.AppTests.Host/Fixtures/TreeViewE2EFixtures.cs Adds the basic text TreeView host fixture.
tests/Reactor.AppTests.Host/FixtureRegistry.cs Registers the new E2E fixtures.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 8/8 changed files
  • Comments generated: 2

Comment thread src/Reactor/Core/Reconciler.Update.cs Outdated
Comment on lines +1875 to +1878
var reusable = new global::System.Collections.Generic.Dictionary<string, WinUI.NavigationViewItem>();
foreach (var item in live)
if (item is WinUI.NavigationViewItem nvi && nvi.Tag is string tag)
reusable[tag] = nvi;
Comment on lines +177 to +180
var reusable = new global::System.Collections.Generic.Dictionary<string, WinUI.NavigationViewItem>();
foreach (var item in live)
if (item is WinUI.NavigationViewItem nvi && nvi.Tag is string tag)
reusable[tag] = nvi;
…ack)

Address PR #454 review:
- Slow-path rebuild now consumes the reuse map entry (Dictionary.Remove)
  when a container is reused, so duplicate sibling keys (duplicate Tags, or
  duplicate Content when no Tag is set) fall through to a freshly created
  container instead of adding the same WinUI item to the collection twice.
- Make the snapshot loops' filtering explicit with .Where()/.OfType().

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codemonkeychris
Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in dd7ce54:

  • Duplicate-key reuse (correctness): the slow-path rebuild now consumes the reuse-map entry via Dictionary.Remove(key, out …) when a container is reused. Duplicate sibling keys — duplicate Tags, or duplicate Content when no Tag is set — now fall through to a freshly created container instead of adding the same WinUI item to the collection twice. Applied to both the legacy arm and the V1 descriptor.
  • Explicit filtering: the snapshot loops now filter explicitly with .OfType<…>() / .Where(…).

Verified: NavigationView self-tests (incl. Desc_NavigationView_MountUpdate) pass with 0 failures, and the NavigationViewInteractionTests E2E guard still passes (and still fails on the pre-fix build).

@codemonkeychris codemonkeychris merged commit 0b24548 into main May 29, 2026
15 checks passed
@codemonkeychris codemonkeychris deleted the fix/navigationview-expand-collapse-rebuild-clobber branch May 29, 2026 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants