Repository navigation
feat(layout): the default node page is a template (B1, part 2) - #5952
Conversation
Overview / Data (BuildDetailsTemplate), the provenance strip WithNodePage composes (ComposeProvenance), and Edit (BuildEditTemplate) are emitted once the viewer's permissions are known and BIND what they show, instead of waiting for the node (and the partition root) on the hub and rebuilding the page from its values on every edit: - Header: title, icon and provenance line bound to a node projection in /data/nodeHeader; a node excluded from the header context hides it through its bound style; object actions chosen from configuration. - Property overview / Edit form: the content type comes from configuration (MeshDataSource, else the type registered for the NodeType); fields bound to the node; the one-way /data mirror is opened in the control's buildup. A hub naming no content type renders its form in the NodeContentForm skeleton slot, the one remaining structure read. - Markdown body: a MarkdownControl bound to /data/nodeBody, hidden while there is none. - Provenance strip: bound to /data/nodeProvenance. Ratchet: LayoutAreaDataBakeSites.allow 67 -> 64 (MeshNodeLayoutAreas 4 -> 1). Pinned by NodePageIsATemplateTest (negative control: all five fail against part 1's head); OverviewMarkdownFreshnessTest now resolves the bound body. Graph.Test 2392/2392. Depends on Systemorph/MeshWeaver.Plugins#2637 (the export renderer resolving bound pointers), which must merge and seal first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 2) 4 files ±0 4 suites ±0 3m 7s ⏱️ +6s Results for commit 74fa02a. ± Comparison against base commit cb01405. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 5)2 685 tests ±0 2 685 ✅ ±0 12m 41s ⏱️ -10s Results for commit 74fa02a. ± Comparison against base commit cb01405. This pull request removes 25 and adds 9 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results 20 files ±0 20 suites ±0 39m 15s ⏱️ -56s Results for commit 74fa02a. ± Comparison against base commit cb01405. This pull request removes 29 and adds 18 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
#5940 and #5946 (the bases) are on main. The data-bake ratchet's TotalBudget stays at main's value (51): this change lowers MeshNodeLayoutAreas.cs from 4 to 1 in test/LayoutAreaDataBakeSites.allow, leaving the allow-file sum at 48. The budget is lowered once, with the last conversion of the series, so that sibling conversions do not conflict on that one line (the guard fails only when the sum EXCEEDS the budget; slack is reported as STALE). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![systemorph-com[bot]](https://avatars.githubusercontent.com/u/77832550?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Part 2 of B1: the default node page becomes a template. Overview and Data emit after a Read/Update permission gate (DistinctUntilChanged) instead of CombineLatest(node, permissions, partitionRoot), and bind what they show: the header (title, icon, provenance line) and markdown body are bound to new hub-side projections in NodePageProjections.cs published through Template's stream Bind; the property form is the configured content type's form with fields bound to the node, and the /data read-mirror moves from a per-render ReplaceDisposable into the control's buildup (MirrorContent); a new NodeContentForm area is the fallback slot for hubs whose configuration names no content type, its shape keyed on the node's $type; Edit and the composed provenance strip follow the same shape per the PR description. NodePageIsATemplateTest pins the bound shapes and follows a live edit, and OverviewMarkdownFreshnessTest now resolves bound pointers the way the GUI does. Checked from the diff: the permission gate, projection purity and disposal, the mirror lifecycle, the ContentForm shape/distinct logic, HTML encoding in the visible part of MetaRowHtml (label and text are HtmlEncoded), $type used only as a distinct-until-changed key or through the content-type registry (no untrusted type activation), and the repository's hard rules — findings below. NOT VERIFIED: the diff is incomplete — the patch of src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs is truncated (first 20000 of 29919 characters), so the tail of MetaRowHtml (everything after 'if (entry.By is { } by)', including the link rendering) and all later hunks of that file (ComposeProvenance/WithNodePage, the Edit template, ProvenanceDataId, any GetContainerStyle overload) went unread; nothing is asserted about them. Also unverifiable from the diff alone: the cross-repo ordering claim (MeshWeaver.Plugins#2637 must resolve document-export pointers before this goes live), the ratchet budget totals, the claimed 2392/2392 test run, and whether the framework permission-gates direct remote opens of a registered area (the basis of the should-fix finding).
Findings: 1 blocking · 1 should-fix · 1 question · 2 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
question src/MeshWeaver.Graph/NodePageProjections.cs
The PR discloses that NodePageProjections.cs sits in the LayoutAreaDataBake ratchet's blind spot — the text scan cannot see a load that lives in another file — and imposes a narrow contract on the file (projections and the mirror, never a control). Nothing visible in the diff enforces that contract: the allow-file change only re-counts src/MeshWeaver.Graph/MeshNodeLayoutAreas.cs (4 to 1), and NodePageProjections.cs is not in the inventory at all, so a future change adding a control or a render-time load to that file would evade the guard silently. Is there, or can there be, something that fails when the file outgrows its contract — an allow-list entry pinning its site count, or a guard test asserting the file holds no controls?
Internal review of a0972bd550a84e9ff78a05ab80d62f199f758dd2 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| .Select(n => n?.Content) | ||
| .Where(c => c is not null) | ||
| .Subscribe(content => host.UpdateData(dataId, content!)); | ||
| host.RegisterForDisposal(context.Area, subscription); |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
New code silences nullability warnings with the null-forgiving operator, which the repository's hard rules ban ("no ! or #pragma to silence a warning"). Sites added by this diff: NodePageProjections.cs line 138 (`.Subscribe(content => host.UpdateData(dataId, content!))` in MirrorContent) and line 152 (`NodeIconPickerDialog.Build(host, node!)` in OpenIconPicker); src/MeshWeaver.Graph/OverviewLayoutArea.cs line 256 (`NodePath = node!.Path` in BuildMarkdownBody — the old statement body's `if (node is null) return null;` let flow analysis prove the dereference, the rewritten expression body cannot, so it adds the suppression); test/MeshWeaver.Graph.Test/NodePageIsATemplateTest.cs (the `(T)(await …)!` cast in the Find helper) and test/MeshWeaver.Graph.Test/OverviewMarkdownFreshnessTest.cs (`resolved.Where(b => b is not null).Select(b => b!)` in Resolve). Each site has a runtime guard the compiler cannot see, so the suppression is mechanically removable — `.Select(n => n?.Content).OfType<object>()` for the mirror, pattern matching elsewhere — and a brand-new file should not carry the pattern forward: the removed OverviewLayoutArea.cs ReplaceDisposable line used the same `content!`, and this rewrite moves that pattern into the new file instead of eliminating it.
There was a problem hiding this comment.
Fixed in 0906086 and 74fa02a — none of the suppressions is left, and no pragma replaced them. MirrorContent: .Select(n => n?.Content).OfType<object>(). OpenIconPicker: .OfType<MeshNode>().Take(1). BuildMarkdownBody: node is not null && MarkdownBodyText(node) is { } body, so flow analysis proves node.Path. Test Find: Walk(…).OfType<T>().Should()…Match(predicate, because) returns the typed control. OverviewMarkdownFreshnessTest.Resolve: resolved.OfType<MarkdownControl>(). The same pattern sat at five further sites you did not list, all in NodePageIsATemplateTest (four DataContext! and a.Area!.ToString()! in Walk); they are gone too (74fa02a: a ContextOf helper that fails naming the control, and OfType<string>() in Walk). A scan of every added line of this PR against main for the operator now returns nothing. Release -warnaserror is clean for MeshWeaver.Graph and MeshWeaver.Graph.Test.
| ? (UiControl?)Controls.Markdown(host.Localize("ui.mdNodeNotFound")) | ||
| : edit | ||
| ? (t.CanEdit ? EditForm(host, t.Node) : null) | ||
| : BuildPropertyOverview(host, t.Node, t.CanEdit)); |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
ContentForm renders the node's property form with no Permission.Read check: it composes host.Hub.GetEffectivePermissions(hubPath) but consumes only Permission.Update, and in overview mode falls straight to BuildPropertyOverview(host, t.Node, t.CanEdit) for any viewer. Every sibling view that renders node content gates Read itself — Overview and ContentData render BuildAccessDenied otherwise — and this view is registered as a top-level area (.WithView(OverviewLayoutArea.ContentFormArea, OverviewLayoutArea.ContentForm) in AddDefaultLayoutAreas), which makes it addressable by area reference like its siblings. If a client can open an area stream directly, a viewer without Read on the node can read its content fields through this form; whether the framework refuses such direct opens could not be verified from this diff. A Read gate matching the siblings' (or a confirmed framework-level gate) closes it.
There was a problem hiding this comment.
Both halves, and the gate is added (0906086).
What the framework does on a direct open: it refuses it. SubscribeRequest is declared [RequiresPermission(Permission.Read)] (src/MeshWeaver.Data.Contract/Messages.cs), and the delivery pipeline (src/MeshWeaver.Hosting/Security/AccessControlPipeline.cs) evaluates that attribute before any handler: a viewer without Read on the node gets Access denied: user '…' lacks Read permission on '…' and no area stream. So with the standard check, opening NodeContentForm by reference without Read never reached this view.
Why the view gates anyway: that pipeline verdict is not always the viewer's effective permission on the node — a denied check is re-decided by the node type's own INodeTypeAccessRule, and hub-level rules (WithPublicRead) short-circuit it — which is exactly why Overview and ContentData render BuildAccessDenied themselves. ContentForm now does the same: it projects Read and Update from GetEffectivePermissions, and without Read returns MeshNodeLayoutAreas.BuildAccessDenied before the node is looked at.
Pinned: new ContentFormIsReadGatedTest (2 cases) drives the view through a seam that takes the permission stream — same node, hub and viewer, Permission.None vs Permission.Read: the denied arm shows the access-denied label and no control of the form (neither node-bound nor bound to the area's content mirror); the control arm renders the form. Negative control: with the read arm removed the denied case FAILS (1 of 2), restored 2 of 2. Not established: I did not build a mesh where the pipeline admits the subscribe while the effective permission lacks Read, so the end-to-end path through INodeTypeAccessRule is argued from the code, not measured.
| if (node?.Icon is { } raw && MeshNodeImageHelper.IsFluentIconName(raw)) | ||
| return (raw, "36px", TileStyle); | ||
| return (iconValue, "34px", TileStyle); | ||
| } |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
IconOf's doc comment promises "a glyph at 30px", but the glyph branch returns (iconValue, "34px", TileStyle). Comment and code disagree; BuildClickableIcon's own sizes are outside the visible part of this diff, so the intended value cannot be read here — whichever it is, the two should match.
There was a problem hiding this comment.
Comment corrected to the code (0906086): IconOf draws a glyph at 34px. BuildClickableIcon draws its glyph as inline HTML text at font-size: 30px; here the value is an IconControl width, which is the size this PR's rendering was built and reviewed with, so I changed the sentence rather than a rendered size I cannot look at from here. The comment now says both numbers and why they differ. Not established: a side-by-side render of the two glyph sizes.
| @@ -71,46 +72,124 @@ public static UiControl BuildPropertyOverview(LayoutAreaHost host, MeshNode node | |||
| var dataId = EditLayoutArea.GetDataId(node.Path); | |||
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
After the rewrite, BuildPropertyOverview computes dataId (line 72) and boundContext (line 73) and then returns PropertyOverview(host, node.Path, contentType, canEdit), which recomputes both — the two locals are dead. Removing them, or passing them through to PropertyOverview, resolves it.
There was a problem hiding this comment.
Removed (0906086): BuildPropertyOverview no longer computes dataId and boundContext, nor repeats the comment block that belongs to them — it ends in return PropertyOverview(host, node.Path, instance.GetType(), canEdit);, and PropertyOverview is the one place both are derived.
… operators; dead locals and a stale size comment - OverviewLayoutArea.ContentForm rendered the property form for a viewer without Read (it read the effective permissions and consumed only Update) while Overview and Data render the access-denied view. It is a registered top-level area, so it gates Read itself now; ContentFormIsReadGatedTest pins both arms through the permission-stream seam (negative control: read arm removed, the denied case fails). - The five null-forgiving operators are gone: OfType in MirrorContent, OpenIconPicker and the freshness test's Resolve, a null check the compiler can follow in BuildMarkdownBody, and a typed wait in the test's Find helper. - BuildPropertyOverview no longer computes two locals PropertyOverview recomputes. - IconOf's comment states the 34px the code draws a glyph at. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…either The same pattern the review named at five sites was left at five more in NodePageIsATemplateTest (four DataContext dereferences and the Walk helper's area name). ContextOf fails naming the control, and Walk filters with OfType. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
#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>
…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>
1 participant
What
This is B1 part 2: the conversions that part 1 (#5946) left out because of the export dependency. The default node page is now a template (
Doc/GUI/DataBinding→ "Templates first, data later"). It is emitted without waiting for the node, and it binds what it shows.OverviewandData(BuildDetailsContent→BuildDetailsTemplate)CombineLatest(node, permissions, partitionRoot), then the whole page rebuilt from the node's values on every edit/data/nodeHeader(NodePageProjections.Header). The header-context exclusion is a bound style. The object actions come from configuration. Property form: the configured content type's form (ConfiguredContentType), with its fields bound to the node. Markdown body: bound to/data/nodeBody, hidden while there is none.ComposeProvenanceCombineLatest(node, page)/data/nodeProvenance.OverviewLayoutArea.BuildPropertyOverviewReplaceDisposablenode-stream mirror per renderNodePageProjections.MirrorContent) and disposed with the area.BuildPropertyOverviewTemplate(host, contentType, canEdit)added.BuildEditNodeContent→BuildEditTemplateCombineLatest(node, permissions)On a hub whose configuration names no content type, the form renders in the new
NodeContentFormskeleton slot (OverviewLayoutArea.ContentForm). Its SHAPE can only come from the node's$type. That slot is a structure read. It stays in the ratchet inventory (OverviewLayoutArea.cs 1, where it replacesBuildPropertyOverview).All public module-facing helpers keep their signatures and behaviour:
BuildHeader(host, node, …),BuildMetaRow,BuildPropertyOverview(host, node, canEdit),BuildMarkdownBody,BuildTitle. No public surface is removed.Not identical, stated plainly:
IconControl, which handles the four icon shapes. Its sizes match the old ones, but an image no longer getsobject-fit.MarkdownControlwhere there used to be no control.Ratchet blind spot, disclosed: the projections live in
NodePageProjections.cs. They are the sanctioned "rows computed on the hub →/data, bound by pointer" shape (Template's streamBind). The text scan cannot see a load in another file, so that file's contract is narrow: projections and the mirror, never a control. This is documented inDataBinding.md.Evidence
NodePageIsATemplateTest(5 tests) covers:metaHtml;14267ae8ee), all 5 fail. Each times out on its first wait.OverviewMarkdownFreshnessTestnow resolves the bound body the way the GUI does. Its assertions are unchanged: current source, edit, clear, legacy HTML, and no body for a record.MeshWeaver.Graph.Test: 2392/2392 passed.LayoutAreaDataBakeRatchetGuardandNodePageProvenanceGuardare green.-warnaserrorwith 0 errors onMeshWeaver.Graph,MeshWeaver.Graph.TestandMeshWeaver.Documentation.Test.LayoutAreaDataBakeSites.allowgoes from 67 to 64 (MeshNodeLayoutAreas.csfrom 4 to 1; the remaining unit isNodeTypeCatalog, out of scope).TotalBudgetgoes from 67 to 64.Doc/GUI/DataBinding→ "The default node page itself (converted)" replaces part 1's "still to convert" note.Ordering: the satellite half lands FIRST (the usual order is inverted)
The document export reads
MarkdownControl.Markdown/HtmlControl.Data. With this PR those are pointers, so exports of pages embedding these areas would come out EMPTY until the export renderer resolves pointers.Systemorph/MeshWeaver.Plugins#2637 must merge AND reach a sealed/passed set before this goes live. This PR is therefore held as a draft:
auto-arm.ymlre-arms any non-draft PR. It is also stacked on #5946, which is stacked on #5940. Retarget it tomainonce those merge.Pairs-with: none — no public type or member removed (
BuildDetailsContentwas internal,BuildEditNodeContentprivate); the dependency runs the other way: Systemorph/MeshWeaver.Plugins#2637 lands first.Deploy
Per-node hubs bind their configuration once, so already-running node activations keep serving the old page until they are recycled. The roll replaces the activations on the pods it rolls. No
recycleis needed beyond the roll.🤖 Generated with Claude Code