Repository navigation
feat(layout): a NodeType definition's catalog is a template — the instance list in a skeleton slot (B1 follow-up) - #5980
Merged
Conversation
…tance list renders in a skeleton slot Search on a NodeType definition subscribed to the node and emitted nothing until it answered, then built the MeshSearch from DefaultNamespace / RestrictedToNamespaces. The page is now emitted at once (breadcrumbs + a NodeTypeInstances slot with the skeleton, carrying the query string); only the slot reads the definition, through the pure NodeTypeCatalogQuery.From, and re-renders when the query or the create link changes. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Test Results (shard 2) 4 files ±0 4 suites ±0 3m 15s ⏱️ +24s Results for commit 1f39ae8. ± Comparison against base commit 1d5a337. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Test Results (shard 5) 4 files ±0 4 suites ±0 12m 53s ⏱️ +7s Results for commit 1f39ae8. ± Comparison against base commit 1d5a337. This pull request removes 25 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Test Results (shard 3) 4 files ±0 4 suites ±0 8m 58s ⏱️ -49s Results for commit 1f39ae8. ± Comparison against base commit 1d5a337. This pull request removes 1 and adds 4 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Test Results 20 files ±0 20 suites ±0 39m 16s ⏱️ - 1m 4s Results for commit 1f39ae8. ± Comparison against base commit 1d5a337. This pull request removes 30 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
#5952 is on main. The data-bake guard's TotalBudget stays at main's value here (slack is a STALE warning); the allow-file carries this PR's lowered line. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
marked this pull request as ready for review
October 2, 2026 16:55
rbuergi
enabled auto-merge
October 2, 2026 16:55
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Converts a NodeType definition's Search page into a template emitted at once - the breadcrumb trail plus a new NodeTypeInstances skeleton slot carrying the page's query string - and moves the definition-derived instance list (hidden query and create link, refactored into the pure NodeTypeCatalogQuery.From and BuildNodeTypeSearch) into that new hidden area registered on every node hub, re-rendering only when the query record changes (DistinctUntilChanged on the record). The deleted NodeTypeCatalog's decision logic is preserved verbatim inside From: same hidden-query branches, same Uri-escaped /create link, the unused createNs dropped. The doc row is rewritten to match and the file is removed from the ratchet allow-list, disclosed in the PR body. Four executing tests cover the template's static shape, From's decision cases, the rendered Search area emitting the slot, and the slot following a later definition edit. Checked: the moved logic against the deleted code line by line, the changed product code's Rx shape (no async/await/Task; observables throughout), framework controls only, and no new injection/secrets/untrusted-$type surface (query-string and href building are unchanged from the old code). Could not verify: InstanceCatalog, BuildCatalog, ReadCatalogOptions and WithBreadcrumbs lie outside the diff, so the null-node fallback's MeshSearchRenderMode.GraphNavigator default cannot be compared with the InstanceCatalog call it replaces (filed as a question); the ratchet scanner's pattern behind the allow-list removal (also filed); and the PR body's test and CI claims (46/46, ratchet guard 3/3, the negative control) - the item carries no CI results. The PR is stacked on #5952 and retargets to main on merge; the reviewed diff contains only this change's four files. No blocking findings: one should-fix and two questions.
Findings: 0 blocking · 1 should-fix · 2 question · 0 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
question src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs
The null-node fallback in NodeTypeInstances builds BuildCatalog(hubPath, ReadCatalogOptions(host, MeshSearchRenderMode.GraphNavigator)) under the comment 'No definition to read (yet): the ordinary catalog, as before', but the branch it replaces - the deleted 'return InstanceCatalog(host, hubPath);' at the end of the old NodeTypeCatalog - called InstanceCatalog. InstanceCatalog's body is outside this diff, so the equivalence cannot be checked from the change: it holds only if InstanceCatalog renders exactly BuildCatalog with a GraphNavigator default, in particular the same default render mode. If InstanceCatalog defaults to a different mode, the catalog shown while no definition is readable changes shape. Filed as a question because the claim 'as before' is not verifiable from the diff alone.
question test/LayoutAreaDataBakeSites.allow
This removes src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs from the ratchet's inventory while the same PR adds NodeTypeInstances, which still reads the node (host.Workspace.GetMeshNodeStream()) and decides its controls from it. The PR body and the rewritten doc row both state the reason: the slot builds its controls through the pure helpers BuildNodeTypeSearch and BuildCatalog, so the text scan no longer counts the read. Both disclosures match the diff as far as it shows, but the scanner's pattern is not part of the diff, so whether 'controls built through pure helpers' is a sanctioned exit from the ratchet or a blind spot cannot be verified here. The doc row contrasts this with NodeContentForm, which per that row remains a counted STRUCTURE read; this file now leaves the inventory, so the ratchet no longer tracks the slot at all.
Internal review of 093c6fce7b936b657f4321b6db030561cbeea6e2 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| // ordinary node's catalog — the hot path — is therefore a template emitted at once; only | ||
| // a definition's catalog reads its node, because its query is built from the definition. | ||
| // configuration, which the activation funnel records as NodeTypePathHolder. Both branches | ||
| // are templates emitted at once: a definition's instance list — whose query is computed |
Contributor
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The comment edit duplicates a clause. The unchanged line above the change reads '// configuration, which the activation funnel records as NodeTypePathHolder. Every', and the first added line begins '// configuration, which the activation funnel records as NodeTypePathHolder. Both branches'. In the new file the comment therefore carries the same 'configuration, which the activation funnel records as NodeTypePathHolder' clause twice in a row with a dangling 'Every' between them - the added lines were written to replace the '... Every' line, but that line was kept as context. Only the duplication is wrong; the new comment's substance is accurate.
Contributor
Author
There was a problem hiding this comment.
Correct — the old '… NodeTypePathHolder. Every' line was meant to be replaced and was left in as context, so the comment in MeshNodeLayoutAreas.cs says the clause twice with a dangling 'Every'. Comment-only, so it is fixed forward in the follow-up PR that collects today's deferred review items (the same one that carries the Todo planning fix from #5973), not with another head and suite run here. I will link it on this thread.
Contributor
Author
There was a problem hiding this comment.
Correction to my reply above — I agreed without opening the file, and the finding does not hold. On the head the comment reads the clause ONCE: grep -n 'records as NodeTypePathHolder' src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs gives a single line (1478, '… NodeTypePathHolder. Both branches'); the old '… NodeTypePathHolder. Every' line was removed by the change, it only appears as the - side of the hunk. So there is nothing to fix forward for this thread. (This PR got a new head just now for a different reason: it conflicted with main in the data-bake allow-file after #5945 merged — resolved, and TotalBudget lowered to the allow-file's sum, 34.)
…51 -> 34 The conversions merged today (#5946, #5951, #5953, #5952, #5945, #5973) and this one each lowered their allow-file lines while leaving TotalBudget at main's value, so that sibling PRs did not conflict on the constant at every landing. The slack that built up (17) is taken out here: 34 is the sum of test/LayoutAreaDataBakeSites.allow on the merged tree. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Converts a NodeType definition's Search page into a template emitted at once - the breadcrumb trail plus a new NodeTypeInstances slot with a skeleton spinner, carrying the Search area's reference id (the page's query string) - and moves the definition-derived instance list into that new hidden area, registered on every node hub: the pure NodeTypeCatalogQuery.From (the deleted NodeTypeCatalog's decision logic preserved verbatim - the same hidden-query branches, the same Uri-escaped /create link, the dead createNs variable dropped), then DistinctUntilChanged on the record, then the pure BuildNodeTypeSearch, so the slot re-renders only when the query or the create link changes and a later edit of the definition reaches an open page. The doc row is rewritten to match, the file leaves the ratchet allow list with TotalBudget lowered, and the old rendered test is replaced by four executing tests, one of which follows a later definition edit to the new query. Checked: the moved logic against the deleted code line by line; the Rx shape (observables throughout, no async/await/Task outside IIoPool); framework controls only, no '!' and no pragma; no new injection or secret surface (the create link is escaped exactly as before, the query string forwarded through the same reference-id mechanism ReadCatalogOptions already read); the tests execute real assertions. This head also resolves the previous head's comment-duplication finding - the duplicated clause is properly replaced now. Could not verify from the diff: InstanceCatalog, BuildCatalog, ReadCatalogOptions, WithBreadcrumbs and the framework control signatures all lie outside it, so the null-node fallback's 'the ordinary catalog, as before' claim is a question; the ratchet scanner's LoadPattern behind the allow-list exit is a question; and the guard lowers TotalBudget from 51 to 34 (-17) while the diff's only inventory change removes a single count-1 allow-list line and the PR body claims '64 to 63' - the third question. CI results are not on the item, so the body's test-run claims (46/46, ratchet guard 3/3, the negative control) are unverified. No blocking findings: three questions, event COMMENT.
Findings: 0 blocking · 0 should-fix · 3 question · 0 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
question test/LayoutAreaDataBakeSites.allow
This removes src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs (count 1) from the ratchet's allow list while the same change adds NodeTypeInstances to that same file - a method that still reads the node (host.Workspace.GetMeshNodeStream()) and decides its rendered controls from it. The PR body, the rewritten doc row and the method's own doc comment all state the reason: the slot builds its controls through the pure helpers BuildNodeTypeSearch and BuildCatalog, so the scanner's text scan no longer counts the read. The scanner's LoadPattern is not part of this diff, so whether a structure read that routes its control building through pure helpers is a sanctioned exit from the ratchet or a blind spot of the pattern cannot be verified here. The doc row itself contrasts this with NodeContentForm, which remains a counted structure read; after this change the ratchet no longer tracks the slot at all.
Internal review of 1f39ae84d5f308e9191ffeeb4d98c1bf58818b89 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| ? null | ||
| : NodeTypeCatalogQuery.From(node.Path, node.ContentAs<NodeTypeDefinition>(options))) | ||
| .DistinctUntilChanged() | ||
| .Select(query => (UiControl?)(query is null |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The null-node branch renders BuildCatalog(hubPath, ReadCatalogOptions(host, MeshSearchRenderMode.GraphNavigator)) under the comment 'No definition to read (yet): the ordinary catalog, as before', but the branch it replaces - the deleted 'return InstanceCatalog(host, hubPath);' at the end of the old NodeTypeCatalog - called InstanceCatalog. Both InstanceCatalog and BuildCatalog are outside this diff, so the 'as before' claim is not checkable from the change: it holds only if InstanceCatalog(host, hubPath) is exactly BuildCatalog(hubPath, ReadCatalogOptions(host, MeshSearchRenderMode.GraphNavigator)) - the same default render mode, and no breadcrumb trail of its own, since the template already renders the trail and a second one inside the slot would render twice. If InstanceCatalog's default mode differs, the catalog shown while the hub's node is unreadable changes shape from the previous behavior. Filed as a question because the equivalence cannot be verified from the diff alone.
Contributor
Author
There was a problem hiding this comment.
It is exactly that, read from the file on this head. InstanceCatalog (MeshNodeLayoutAreas.cs:1503–1506) is WithBreadcrumbs(BuildCatalog(hubPath, ReadCatalogOptions(host, MeshSearchRenderMode.GraphNavigator)), hubPath). The null-node branch of the slot (line 1547) is the inner expression verbatim — BuildCatalog(hubPath, ReadCatalogOptions(host, MeshSearchRenderMode.GraphNavigator)), same default render mode, same options reader — and it deliberately leaves out WithBreadcrumbs, because the page that hosts the slot is NodeTypeCatalogTemplate, which is itself WithBreadcrumbs(<the slot>, hubPath). Composed, an unreadable node renders trail + graph-navigator catalog once, as InstanceCatalog did; there is no second trail inside the slot. No change.
|
|
||
| /// <summary>The seeded inventory's size. Lower it in the same change that lowers a line.</summary> | ||
| private const int TotalBudget = 51; | ||
| private const int TotalBudget = 34; |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
This diff lowers TotalBudget from 51 to 34, a drop of 17, but the only inventory change the diff itself makes is the removal of the single count-1 allow-list line for src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs, and no other file in the diff removes counted reads - the code change moves one read out of the scanner's pattern (the PR body's own account: 'the scanner no longer counts the file (1 to 0, TotalBudget 64 to 63)'). Neither the base value (51 against the body's 64) nor the delta (-17 against the body's -1) matches that account. If 17 counted lines really left the inventory with this change, the diff does not show where; if they did not, the budget is lowered beyond what the change accounts for - the guard's own rule, 'Lower it in the same change that lowers a line', suggests the drop should be 1. Filed as a question: the diff alone cannot reconcile the three numbers.
Contributor
Author
There was a problem hiding this comment.
The three numbers reconcile through commits that are on this branch but not in this PR's own code change, and the PR body is stale on both (I have not edited it: the runner pool is saturated and a body edit starts runs).
- Base 51, not 64. 64 was the budget on the branch this PR was stacked on when it was written. Since then feat(layout): the default node page's secondary areas are templates (B1, part 1) #5946, feat(samples): Northwind, Article and PythonDemo views are templates (data-binding B4, part 1) #5951, feat(doc): the SocialMedia Post and Profile views are templates (data-binding B4, part 2) #5953, feat(layout): the default node page is a template (B1, part 2) #5952, feat(layout): NodeType + Settings pages are templates — data binds, never bakes (data-binding B2) #5945 and feat(samples): the Todo areas are templates with row-scoped actions (B4 Todo, on #5955) #5973 merged, and main's constant stands at 51 (set by feat(doc): the SocialMedia Post and Profile views are templates (data-binding B4, part 2) #5953).
- Why the constant did not follow the inventory on main. feat(layout): the default node page is a template (B1, part 2) #5952, feat(layout): NodeType + Settings pages are templates — data binds, never bakes (data-binding B2) #5945 and feat(samples): the Todo areas are templates with row-scoped actions (B4 Todo, on #5955) #5973 each lowered their allow-file lines but were merged with
TotalBudgetleft at main's 51 — on purpose, by me: sibling conversions each rewriting that one constant conflicted with each other at every landing, and the guard treats budget above the sum as slack (it only fails when the sum EXCEEDS the budget and reports the rest as STALE). That left 16 units of slack on main. - 34 = the allow-file's sum on this head. 16 units of inherited slack plus this PR's own 1 (the
MeshNodeLayoutAreas.csline) is the 17. The tidy is its own commit,test(doc): the data-bake budget is the inventory again — TotalBudget 51 -> 34, and says so.LayoutAreaDataBakeRatchetGuardruns 3/3 on this head; lowering the budget cannot hide anything — it makes the guard stricter.
So the rule 'lower it in the same change that lowers a line' was bent for three merges and is restored here; nothing left the inventory that the allow-file does not show.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Data-binding follow-up:
MeshNodeLayoutAreas.NodeTypeCatalog, the last unit B1 part 2 (#5952) left inMeshNodeLayoutAreas.cs.Root cause
Searchon a NodeType DEFINITION returnedGetMeshNodeStream().Select(node => …). It emitted nothing until the definition node answered, then built theMeshSearchfromDefaultNamespace/RestrictedToNamespaces. So the page showed only the area spinner until the owning hub answered.Design (rule (c), the documented last resort)
The instance list's hidden query and its create link are COMPUTED from the definition.
MeshSearchView(Blazor and React) readsHiddenQueryas a literal string, not a pointer, so neither (a) bound style variants nor (b) a bound list can express it today.Searchemits the template at once (NodeTypeCatalogTemplate): the breadcrumb trail plus aNodeTypeInstancesslot (Controls.LayoutArea(…).WithSpinnerType(SpinnerType.Skeleton)). The slot carries the page's query string, so?groupBy,?qand the rest still reach the list.NodeTypeInstances(a new hidden area on every node hub) is the only part that reads the node. The decision is the pureNodeTypeCatalogQuery.From(path, definition). It is followed byDistinctUntilChanged, then the pureBuildNodeTypeSearch, so the slot re-renders only when the query or the create link changes. An edit of the definition reaches an open page (before, the area re-rendered on every emission). With no definition, the slot shows the ordinary catalog, as before.Honest about the ratchet: the scanner no longer counts the file (1 → 0,
TotalBudget64 → 63) because the slot builds its controls through pure helpers. It is still a STRUCTURE read, likeNodeContentForm. The doc says so. BindingMeshSearch.HiddenQueryby pointer in the views would remove the slot. That is a possible platform follow-up and is not in this PR.Evidence
NodePageAreasAreTemplatesTest, 4 new tests:EveryViewIsStatic, has noMeshSearchin the frame, has one skeleton slot and forwards the query string;Fromdecisions hold (plain, DefaultNamespace + RestrictedToNamespaces, non-definition);namespace:Tpl/Widget), then FOLLOWS a later definition edit tonodeType:Tpl/Widget namespace:Tpl/Widgets.NodePageAreasAreTemplatesTest,SearchAreaGroupByTestandNodePageIsATemplateTest..Take(1)on the slot's node stream, the 2 rendered tests FAIL at their 36 s convergence waits. They are green again once reverted.LayoutAreaDataBakeRatchetGuardpasses 3/3, and the documentation link tests pass. No new user-visible strings, so there is noMirror-sync:line.dotnet build -c Release -warnaserror: 0 errors and 0 warnings on MeshWeaver.Graph, MeshWeaver.Graph.Test and MeshWeaver.Documentation.Test.Doc:
Doc/GUI/DataBinding→ "The default node page's secondary areas (converted)", theSearchrow. No What's New file.Deploy
No in-mesh source changes. The code is compiled into
MeshWeaver.Graph, so a roll replaces it. An open NodeType definition hub (any…/<TypeName>node of nodeTypeNodeType) keeps serving the old Search until it is next activated. Recycle a definition address only if it still answers the old way.Pairs-with: none — no public type or member removed (
NodeTypeCatalogwas private;NodeTypeInstancesArea/NodeTypeInstancesare additions)🤖 Generated with Claude Code