Repository navigation
fix(#3406): the navigation rail preserves the supplier's order - #3414
Conversation
SuppliedNavigationRail.Plan partitioned the supplied entries into two buckets — Pages (no children) and Groups (children) — and Render walked them with two loops, emitting Home, then every Page, then every Group. The supplier's order survived INSIDE each bucket and was lost BETWEEN them, so an entry with children could never precede one without. Measured on a live course: four lessons carrying Order 1-4 rendered tenth to thirteenth, behind every leaf page, because each had an exercise, solution, quiz and documents folder beneath it. No Order value could fix it — the numbers were already right. The plan now carries ONE ordered sequence, Rail.Items, whose element is a RailLink or a RailGroup (a closed hierarchy under a new abstract RailItem), and Render walks it once. Having children decides WHAT an entry becomes, never WHERE it goes. Both container renderers — Blazor's NavMenuView and the React NavMenu skin — emit a container's areas in declaration order, and WithNavLink / WithNavGroup both append to that one ordered list, so the plan was the only place the order was lost. Falsification: reproducing the bucket ordering on the fixed API shape, with the test file untouched, fails 2 of 13; the fix passes 13 of 13. Reverting src/SuppliedNavigationRail.cs to main outright leaves the test assembly uncompilable (2x CS0426, RailItem). No user-visible string added — every label in the rail comes from the supplying module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
It removes public nested API members (Rail.Pages/Rail.Groups) without a compatibility shim, which can break external/in-mesh callers that repo-level searches can’t reliably exclude.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Updates SuppliedNavigationRail (MeshWeaver.Graph) so the rendered navigation rail preserves the supplier’s declared entry order even when leaf pages and folders-with-children interleave, addressing #3406.
Changes:
- Replace the previous “Pages then Groups” bucket plan with a single ordered
Rail.Itemssequence ofRailItem(RailLink|RailGroup) and render it in one pass. - Extend unit tests to pin ordering across mixed leaf/group entries and across different “current page” positions.
- Add architecture + What’s New documentation describing the rail contract and the ordering invariant.
File summaries
| File | Description |
|---|---|
| src/MeshWeaver.Graph/SuppliedNavigationRail.cs | Switch plan+render to a single ordered item sequence to preserve supplier ordering across entry kinds. |
| test/MeshWeaver.Graph.Test/SuppliedNavigationRailTest.cs | Add/adjust tests to validate ordering invariants and ensure the fix doesn’t flatten groups. |
| src/MeshWeaver.Documentation/Data/WhatsNew/2026-09-06-a-course-index-follows-the-order-you-gave-it.md | What’s New entry describing the user-visible fix. |
| src/MeshWeaver.Documentation/Data/Architecture/SuppliedNavigationRail.md | New architecture doc capturing the supplier/core contract and the ordering guarantee. |
| src/MeshWeaver.Documentation/Data/Architecture.md | Add the new architecture page to the UI section index. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// <param name="Items">The supplied entries, IN THE ORDER SUPPLIED — a childless entry as a | ||
| /// <see cref="RailLink"/>, one with children as a <see cref="RailGroup"/>. One sequence, not a | ||
| /// bucket per kind: see the class remarks, defect 4.</param> | ||
| public sealed record Rail(RailLink Home, IReadOnlyList<RailItem> Items); |
There was a problem hiding this comment.
Agreed, and done in f3e04f8 — your review and the Public surface (binary compatibility) gate landed on the same risk from two directions, which is a good sign it is real.
Pages and Groups are back as [Obsolete] computed properties over Items, so a caller that reads them still compiles and still gets its entries in rail order. Being derived is the point: the two-bucket model cannot drift back into existence, and the obsolete message names the trap explicitly — concatenating them re-creates the #3406 ordering bug, since that concatenation is literally what the old Render did.
On the sweep: you are right that a repo grep is not the whole answer here. I want to be explicit that I could not close it. search_chunks answered "searched": false (error: search-not-performed, no embedding provider) on both deployments available to me, so per AGENTS.md that is a failed sweep, not a clean one, and I am not citing it. What I do have is two repo-level sweeps that agree — GitHub org-wide code search for SuppliedNavigationRail returns 3 hits, all in this repo, and git grep over the whole MeshWeaver.Plugins origin/main tree including node sources returns 0 — plus the fact that Plan, Render and IsAtOrBelow keep their signatures, so the ordinary in-mesh usage Render(Plan(...)) is untouched either way. The forwarders are what covers the residue I cannot see.
The one thing forwarders cannot cover is the record's primary constructor, which cannot stay at arity 3 alongside a single ordered sequence. That is recorded in scripts/record-signatures.allow with the evidence, and it is a two-commit contract — the entry goes stale fleet-wide the moment this merges, so it gets deleted in the very next change.
Two signals converged on the same risk: the `Public surface (binary compatibility)` gate flagged Rail's primary constructor going 3 -> 2 parameters, and Copilot's review flagged the Pages/Groups removal for in-mesh callers a repo grep cannot see. Both are the same class of consumer — one that compiles at RUNTIME in the portal. Pages and Groups come back as [Obsolete] COMPUTED properties over Items, so a caller that reads them still compiles and still gets the entries in rail order. They are derived, so the two-bucket model cannot drift back into existence, and the obsolete message names the trap: concatenating them re-creates #3406. AGENTS.md's rule for surface that in-mesh code may hold is a forwarder, never a delete. The primary-constructor arity cannot be preserved alongside a single ordered sequence, so Rail is recorded in scripts/record-signatures.allow with the evidence that nothing outside core binds it. That entry is a two-commit contract: it goes stale fleet-wide the moment this merges and must be deleted in the very next change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… self-test count
Two findings from the automatic review.
1. "/issues/{n} may not carry pull_request.merged_at, making EXPIRED
unreachable". The premise is falsified against the live API — #3414 (merged)
resolves to EXPIRED, #3406 (issue) to NOT_A_PR, an absent number to
UNRESOLVED — and /issues/{n} is the ONLY route that tells an issue from a
missing number, which /pulls/{n} answers 404 for alike. But the measurement
lived in prose, so it is now a check: eight resolver self-tests drive
github_resolver with an injected fetch and assert the verdict AND the URLs
requested, plus a fallback to /pulls/{n} for the day the field is absent as a
KEY (never fires today). Even a total loss of the field cannot make the gate
PASS — merged=false + state=closed is ABANDONED, equally red; the fallback
protects the message, not the verdict.
2. "the summary hard-codes +10 but nine cases run". Correct. Fixed by DERIVING
the count — allowance_self_test() returns (failures, ran) — rather than
correcting the literal, and by adding the CLOSED-unmerged case the record gate
had and this one did not. Both gates now run the same ten scenarios.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #3406.
The mechanism
SuppliedNavigationRail.Planpartitioned the supplied entries into two buckets —Pages(nochildren) and
Groups(children) — andRenderwalked them with two loops: Home, then everyPage, then every Group. The supplier's order survived inside each bucket and was lost between
them, so an entry with children could never precede one without.
On the live course in the issue, four lessons carrying
Order1–4 rendered tenth to thirteenth,behind every leaf page, because each had an exercise, a solution, a quiz and a documents folder
beneath it. No
Ordervalue could fix it — the numbers were already right.The plan now carries one ordered sequence,
Rail.Items, whose element is aRailLinkor aRailGroup(a closed hierarchy under a newabstract record RailItemwith aprivate protectedconstructor, so
Render's walk is exhaustive), andRenderwalks it once. Having children decideswhat an entry becomes, never where it goes.
I checked that the plan really is the only place the order was lost:
NavMenuControl.WithNavLinkand
WithNavGroupboth delegate toContainerControl.WithView, which appends to one orderedAreaslist — and both renderers emit that list in order (BlazorNavMenuView→DispatchViewover the container; React
NavMenuSkin→<Children/>). There was never a second re-sortdownstream.
The three properties the class documents as deliberate are unchanged, and each still has its test:
the title is a link and never the collapsible root; an entry with children is a group and its
own first link; the current entry stays in the tree as an active link.
Planstays pure.Not fixed by making the supplier stop declaring children — that buys the ordering by giving up
the collapsible lessons, which #3406 names and rejects.
Falsification
The order test discriminates. Both runs use the unchanged test file:
src/stateSuppliedNavigationRailTest)src/MeshWeaver.Graph/SuppliedNavigationRail.csreverted tomainoutrightCS0426: The type name 'RailItem' does not existThe middle row is the controlled experiment: it isolates the ordering behaviour from the API
shape, so the test is what fails rather than the compiler. Its failure is the reported symptom
exactly —
Four tests are new: two pin the order (one from the index root, one from every standing position, so
expanding a group cannot reorder the rail underneath the reader); one pins that each entry keeps its
own kind, guarding against a "fix" that flattens the groups away; one pins that the rendered menu
gets one area per entry.
MarkdownSideMenuOrderTest+SuppliedNavigationRailTesttogether: 14 passed, 0 failed.Compatibility — what the two gates and the review caught
The first push removed
Rail.Pages/Rail.Groupsoutright. Two independent signals objected, andthey are the same class of consumer: code that compiles at RUNTIME in the portal, which neither
dotnet buildnor a repo grep can see.Public surface (binary compatibility)(check-record-signatures.py) —Rail's primaryconstructor went 3 → 2 parameters.
Pages/Groupsmembers bypass the cross-repo surface gate.Both are addressed in f3e04f8:
PagesandGroupsare back as[Obsolete]computed properties overItems. A caller thatreads them still compiles and still gets its entries in rail order. Being derived is the point:
the two-bucket model cannot drift back into existence, and the obsolete message names the trap —
concatenating them re-creates #3406, which is literally what the old
Renderdid. AGENTS.md's rulefor surface in-mesh code may hold is a forwarder, never a delete.
The primary constructor cannot be preserved alongside a single ordered sequence, so⚠️ That entry is a two-commit
Railisrecorded in
scripts/record-signatures.allowwith the evidence.contract — it goes stale fleet-wide the moment this merges and reddens every open core PR that
touches a
.csfile, so the deletion PR follows immediately behind this one.The sweep evidence, and the part I could not close
scripts/check-type-forwards.py --base origin/main --surface-jsonreports 0 removed, 1 added(
SuppliedNavigationRail.RailItem), so theCross-repo pair (public surface)gate does not fire —Pairs-with: none. Nothing outside core references the type at all: GitHub org-wide code search forSuppliedNavigationRailreturns 3 hits, all in this repo, andgit grepover the wholeMeshWeaver.Pluginsorigin/maintree — node sources included — returns 0.Edu'sEduCourseNavigationProviderand its in-mesh tests are on the supplier side(
NodeNavigation/NodeNavigationEntry, both untouched); they assert the order they hand over,which this change now actually renders.
search_chunksanswered"searched": false(
error: search-not-performed— no embedding provider) on both deployments available to me, soper AGENTS.md that is a failed sweep. The forwarders above are precisely what covers that residue;
Plan,RenderandIsAtOrBelowalso keep their signatures, so the ordinary in-mesh usageRender(Plan(...))is untouched either way.Verification
dotnet build src/MeshWeaver.Graph -c Release -warnaserror --no-incremental0 Warning(s) 0 Error(s), 12.4 s (real compile, not a no-op)dotnet build test/MeshWeaver.Graph.Test -c Release -warnaserror0 Error(s)— noCS0618from the new[Obsolete]membersdotnet build src/MeshWeaver.Documentation -c Release -warnaserror0 Error(s)dotnet build test/MeshWeaver.Documentation.Test -c Release -warnaserror0 Error(s)dotnet build test/MeshWeaver.Layout.Test -c Release -warnaserror0 Error(s)python3 scripts/check-record-signatures.py --base origin/main✓ no binary-breaking primary-constructor changes (1 allow entry, all still needed)DocumentationLinkIntegrityTest+WhatsNewEntryIntegrityTesti18n
No user-visible string was added or changed. Every label in the rail — the heading, each entry,
each group — comes from the supplying module's
NodeNavigation, and this change only reorders whatis already there. The two new strings are a
NotSupportedExceptionmessage on a branch unreachableby construction and two
[Obsolete]messages: both developer diagnostics, not UI. No catalog keyneeded in
strings.{en,de}.json, and nothing to sync toMeshWeaver.Plugins/clients/react/src/i18n/.Work product
Per the conserve-work-products rule, and as #3406 asks: the finding's durable home is
src/MeshWeaver.Documentation/Data/Architecture/SuppliedNavigationRail.md(
Doc/Architecture/SuppliedNavigationRail) — the supplier contract, the four properties coreguarantees, the ordering invariant and why the plan is a pure record. Linked from the Architecture
index's UI row. Plus a
Category: FixWhat's New entry.