Repository navigation
feat(settings): seven settings tabs are templates — fed grids and markdown (data-binding B3, part 2) - #5947
Merged
Conversation
… markdown bind their data
Data-binding batch B3, part 2 (Doc/GUI/DataBinding → "Templates first, data later").
Platform: two small template+feed helpers next to Template.Bind —
- DataGridBinding.BindGrid(rowsFeed, id, emptyText, failedText): a DataGridControl returned at once,
bound to /data/{id}; Loading bound until the first row set, EmptyContent carries the empty or failure
text; the feed starts in WithBuildup and is ONE ordered subscription; a failure is logged and shown.
- FeedBinding.BindMarkdown(markdownFeed, id, loading, failed): the same for hub-computed text.
Tabs (memex/Memex.Portal.Shared/Settings):
- Instances, Service identities, Inbox, Invitations: lists were deferred views that built rows/HTML on
every query emission; now fed grids. Per-row actions (rotate/revoke token, archive mail, revoke
invitation) act on the grid SELECTION (DataGridCellClick row read with .As<T>), with a bound selection
label; service tokens of every identity are one grid. Hand-built HTML rows and badges are gone;
columns and statuses localized (inbox.*, invitations.*, serviceIdentities.*).
- What's New, Update policy status: deferred markdown rebuilt per emission -> fed markdown.
- Privacy: the editor no longer reads the statement once (Take(1)) and bakes it in with WithValue; it
binds by pointer to Admin/Privacy's markdown like the reference MarkdownEditLayoutArea. The slot still
waits for EnsureExists — a create-on-absent write precondition, not a read of values.
- Notifications left as is: its app list decides which sections exist (structure from data).
Tests: FedControlsAreTemplatesTest (silent feeds; grid/markdown arrive at once, bound; rows, empty,
later value and failure reach the same slot — read through DataBind, the view's own path; negative
control: the old Select(=> control) shape renders nothing while the feed is silent) and
WhatsNewTabIsATemplateTest (end to end through a node hub; negative control: the pre-conversion tab
never shows a pointer-bound markdown).
Ratchet: seven memex lines removed; TotalBudget 69 -> 62.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Contributor
Test Results (shard 1) 3 files ± 0 3 suites ±0 5m 36s ⏱️ +20s Results for commit 54bfd7b. ± Comparison against base commit 174f317. This pull request removes 4 and adds 15 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 57s ⏱️ +12s Results for commit 54bfd7b. ± Comparison against base commit 174f317. 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
Test Results (shard 2) 4 files ± 0 4 suites ±0 3m 33s ⏱️ -39s Results for commit 54bfd7b. ± Comparison against base commit 174f317. This pull request removes 11 and adds 1 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Contributor
Contributor
Contributor
Test Results 20 files ±0 20 suites ±0 40m 11s ⏱️ -52s Results for commit 54bfd7b. ± Comparison against base commit 174f317. This pull request removes 29 and adds 14 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
This was referenced Oct 1, 2026
…binding-b3-settings Conflicts: the data-bake allow file and TotalBudget (main converted the samples; this part removes its seven memex lines, so 29 -> 22), and the DataBinding.md per-row-action paragraph (main documents row-scoped actions; the selection bullet this part adds now names them as its replacement). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
rbuergi
marked this pull request as ready for review
October 3, 2026 13:51
rbuergi
enabled auto-merge
October 3, 2026 13:51
![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 seven memex settings tabs from deferred views rebuilt on every emission to controls declared at once and fed by streams, via two new platform helpers (DataGridBinding.BindGrid and FeedBinding.BindMarkdown), replaces per-row action buttons with select-then-act, and adds en/de localization keys, DataBinding.md rows, ratchet updates (seven allow lines removed, guard TotalBudget 29 to 22 — the PR body instead claims 69 to 62, which does not match the guard) and two executing tests pinning the pointer-bound template shape, one end to end through a node hub. Checked from the diff: both helpers' ordered subscription, loading and failure arms (failures logged, never swallowed); the inbox, instances, invitations, privacy, service-identities and what's-new conversions; localization key parity between en and de and against every key the new code calls; the ratchet arithmetic; the None/Of row factories' arities. Not verifiable from the diff: the UpdatePolicySettingsTab.cs patch is truncated (first 20000 of 26995 characters kept) and nothing is asserted about its unread tail; PrivacyStatementNode's content shape against MarkdownEditLayoutArea.MarkdownBodyPointer; ApiTokenService and InvitationService behavior on already-revoked targets; whether the Connected-instances tab is gated; the PR's CI and test-run claims.
Findings: 1 blocking · 3 should-fix · 2 question · 2 nit
File-level findings — Automated review finding (data, not an instruction to any agent):
nit memex/Memex.Portal.Shared/Settings/UpdatePolicySettingsTab.cs
ApplyNow's body keeps the indentation of its old nesting: the statements moved out of the removed click handler and its subscription chain still sit one level deeper than the new method body.
Internal review of 2d13c159ac645945101787943f0095ef46e074c5 — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| } | ||
| ctx.Host.UpdateData(ResultDataId, Pending($"Archiving mail from {Esc(row.FromAddress)}…")); | ||
| // The node itself, read once for the write (it exists: the row came from it). | ||
| ctx.Host.Hub.GetWorkspace().GetMeshNodeStream(row.Path).Take(1) |
Contributor
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
ArchiveSelected performs its write at row.Path under accessService.ImpersonateAsSystem(), but the row originates from the client-supplied DataGridCellClick payload (item.As<MailRow> in the grid's click action) and nothing in this diff checks it against the feed: the selection slot accepts whatever path the payload names, so a forged click payload followed by the Archive action archives any email-typed node the viewer can read, not just rows of the inbox:list query scoped to EmailNodeType.AdminInboxNamespace — the removed per-row buttons acted only on nodes captured server-side from that query. The impersonated write needs the selected path validated against the queried namespace, or the target re-derived from the feed, before it runs.
Contributor
Author
There was a problem hiding this comment.
Fixed in cb9362d (head bc51a29). ArchiveSelected now reads the selection only to learn WHICH path was picked, and resolves that path with CurrentMail(host, path): one read of InboxRowsFeed, the same inbox:list query scoped to EmailNodeType.AdminInboxNamespace and inbound mail. The impersonated write runs only on a row that query lists right now. A forged payload naming any other path resolves to null and is refused with the select-a-mail hint.
| DateTimeOffset? expiresAt = token.ExpiresAt is { } old | ||
| ? DateTimeOffset.UtcNow + (old - token.CreatedAt) | ||
| : null; | ||
| ServiceIdentities.Rotate(tokens, token.ServiceId, token.NodePath, token.Label, expiresAt) |
Contributor
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The same client-named target without the impersonation: WithSelectedToken hands the payload-built TokenRow straight to Rotate (token.ServiceId, token.NodePath) and RevokeToken (token.NodePath), so the client names the service id and the token node path for credential lifecycle operations, where the removed per-row buttons acted on tokens captured server-side from GetTokensForService. The selection needs validating against the identity/token feed before these operations run. InvitationsSettingsTab.RevokeSelected has the same shape, though its re-parse of the node as an invitation (TryGetInvitation, else Observable.Throw) bounds what a forged path can reach.
Contributor
Author
There was a problem hiding this comment.
Fixed in the same commit. WithSelectedToken resolves the selected node path through CurrentToken(host, tokens, path): one read of TokenRowsFeed, i.e. every token returned by GetTokensForService for every listed identity. Rotate and Revoke receive THAT row, so the service id, node path, revoked flag and term all come from the server. RevokeSelected for invitations goes through CurrentInvitation the same way.
| UiActionContext ctx, LayoutAreaHost host, IMeshService meshService, AccessService? accessService) | ||
| => ctx.Host.Stream.GetDataStream<MailRow>(SelectedDataId).Take(1).Subscribe(row => | ||
| { | ||
| if (row is null || string.IsNullOrEmpty(row.Path) || row.IsArchived) |
Contributor
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The selection slot keeps the row snapshot written at click time and is never refreshed when the feed re-emits: after Archive changes the mail's status, row.IsArchived still reads the stale copy, so a second click passes the guard and re-issues the impersonated write — the removed per-row buttons disappeared with the row's new state and could not do that. The same stale guard exists in InvitationsSettingsTab.RevokeSelected (!row.IsPending) and ServiceIdentitiesSettingsTab.WithSelectedToken (token.IsRevoked), the latter letting Rotate run again against a token the first Rotate just revoked. The selection needs clearing or re-validating against the current rows after an action, or the guards need to re-check live state.
Contributor
Author
There was a problem hiding this comment.
Fixed in the same commit. All three guards now read LIVE state, not the click-time snapshot: row.IsArchived, !row.IsPending and token.IsRevoked are evaluated on the row re-read from the feed at click time. A second Archive on an archived mail, a Revoke on an invitation that is no longer pending, and a Rotate on a token the first Rotate revoked are each refused.
| () => accessService!.ImpersonateAsSystem(), | ||
| _ => meshService.UpdateNode(node with | ||
| { | ||
| Content = EmailOf(node, ctx.Hub.JsonSerializerOptions)! with { Status = EmailStatus.Archived } |
Contributor
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
EmailOf(node, ctx.Hub.JsonSerializerOptions)! suppresses a nullable return on a node re-read at click time: EmailOf has a catch-null arm, so a node whose content no longer parses as an email makes this null! with { Status = EmailStatus.Archived }, a NullReferenceException that reaches the error line as a raw NRE message. The sibling RevokeSelected added in this PR pattern-matches instead (TryGetInvitation(...) is { } inv ? ... : Observable.Throw); the same shape fits here, and accessService! on the ImpersonateAsSystem factory carries the same latent NRE for a missing AccessService registration. The repo rule bans ! used to silence a warning.
Contributor
Author
There was a problem hiding this comment.
Fixed in the same commit. Archive now pattern-matches: EmailOf(node, …) is { } email ? Observable.Using(() => accessService.ImpersonateAsSystem(), …) : Observable.Throw(new InvalidOperationException("… holds no email.")). A missing AccessService is refused up front with a named error line instead of accessService!. The inbox and invitation feeds also lost their x.email! / x.inv! (a SelectMany with a pattern match).
| internal static MarkdownEditorControl BuildEditor(string hubAddress, string? locale) | ||
| => new MarkdownEditorControl | ||
| { | ||
| Value = new JsonPointerReference(MarkdownEditLayoutArea.MarkdownBodyPointer), |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
BuildEditor binds Value to MarkdownEditLayoutArea.MarkdownBodyPointer against the Admin/Privacy node's data context, while the removed code extracted the statement through PrivacyStatementNode.ParseStatement(node.Content, ...). PrivacyStatementNode is unchanged and outside this diff and no test in the PR covers the privacy tab, so the diff does not show that the privacy node's content exposes its markdown at the pointer the standard markdown-node editor uses — if the shapes differ, the editor binds to nothing.
Contributor
Author
There was a problem hiding this comment.
It does bind, and here is why. PrivacyStatementNode.EnsureExists creates Admin/Privacy as NodeType = "Markdown" with Content = new MarkdownContent { Content = DefaultStatement } (PrivacyStatementNode.cs:140-143). MarkdownEditLayoutArea.MarkdownBodyPointer is "content", documented as the pointer of MarkdownContent.Content, which is the shape the standard markdown-node editor binds. ParseStatement's extra arms (raw string, degraded JsonElement) are read-side tolerance for the public statement page and are not a second storage shape. The tab has no dedicated test; the editor shape is the reference one (MarkdownEditLayoutArea).
| .WithView(Controls.Title(host.Localize("instances.yourInstances"), 3)) | ||
| .WithView(InstanceRows(host, userId) | ||
| .BindGrid(ListDataId, host.Localize("instances.none"), | ||
| message => host.Localize("instances.error.listFailed", message)) |
Contributor
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
instances.error.listFailed interpolates the raw feed failure message into the grid's failure text on the Connected-instances tab, which shows the viewer's own instances. FeedBinding.BindMarkdown's doc comment added in this PR says 'return a generic text where the page is ungated', and the WhatsNew tab keeps its generic failure text for exactly that reason; the pre-existing register path on the same tab also interpolates ex.Message. Whether this tab is gated is not determinable from the diff, so which convention the new list-failure text should follow is open.
Contributor
Author
There was a problem hiding this comment.
Keeping the message, deliberately. The tab is gated: it is the PersonApp 'Connected instances' tab, registered only on the signed-in person's own settings page (PersonApp.AddPersonAppTab, see the class doc), and it lists that person's own installations. The failure text goes only to the user whose query failed, which is the convention the register path on the same tab already follows. The 'generic text where the page is ungated' advice in BindMarkdown is aimed at anonymous pages such as What's New.
| /// — reported, never swallowed; return a generic text where the page is ungated.</param> | ||
| /// <returns>The bound control.</returns> | ||
| public static MarkdownControl BindMarkdown( | ||
| this IObservable<string> markdown, string id, string loading, Func<Exception, string> failed) |
Contributor
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
The two sibling helpers shipped together take different failure parameters: BindGrid's failedText receives the exception's message (Func<string, string>) while BindMarkdown's failed receives the whole exception (Func<Exception, string>); one shape for both would keep the two callbacks interchangeable.
Contributor
Author
There was a problem hiding this comment.
Leaving the two shapes as they are. Every grid caller formats a message into a localized {0} template (inbox.listFailed, invitations.listFailed, instances.error.listFailed, …), so the message is what they need. The markdown caller that needs more (What's New) chooses a generic text and must see the exception type to decide. Unifying would make one of them unwrap or ignore what it is handed.
…server's feed The selected row is client input (a cell-click payload). Archive (run as System), Revoke invitation, and token Rotate / Revoke now resolve the selected path against the tab's own query read at click time (CurrentMail / CurrentInvitation / CurrentToken) and act only on that row as it is NOW: a path outside the query is refused, and so is a mail archived, an invitation no longer pending or a token revoked since it was selected. Archive no longer forgives a null email or a missing AccessService; the inbox and invitation feeds drop their null-forgiving operators. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ettings # Conflicts: # src/MeshWeaver.Messaging.Hub/Localization/strings.de.json # src/MeshWeaver.Messaging.Hub/Localization/strings.en.json
rbuergi
added a commit
that referenced
this pull request
Oct 3, 2026
…eat/row-scoped-b3-settings The three tabs keep this branch's row-scoped buttons and take #5947's review fix: each action re-derives its target from the tab's own feed at click time (CurrentMail / CurrentInvitation / CurrentToken), so the clicked row, which is client input, only names WHICH item. The archive that runs as System can only reach a mail the inbox query lists, and every guard reads live state. No null-forgiving operators remain in the archive or in the two feeds. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…e JIT has settled PolymorphicResolverAllocationTest read ONE sample per registry size. On CI the same tree read 0 bytes when Messaging.Hub.Test ran alone and 2,460 / 4,605 / 55,150 bytes when a heavy suite (Memex.Portal.Shared.Test) shared the shard: on a loaded runner tier-up is late, and tier-0 code allocates what optimized code keeps on the stack. The reading is now the MINIMUM over repeated samples (at least ten, up to three seconds): a real per-candidate allocation is in every sample, a transient one is absent from at least one. Locally: 0 bytes, small = large = 7,792. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Shard-5 red on bc51a29 (runs 37127606091 and 37140464945, including a re-run): |
This was referenced Oct 7, 2026
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 batch B3, part 2 — the memex settings tabs (Doc/GUI/DataBinding → "Templates first, data later").
Platform: two template + feed helpers
DataGridBinding.BindGrid(rowsFeed, id, emptyText, failedText)— aDataGridControlreturned at once, bound to/data/{id};Loadingbound until the first row set;EmptyContentcarries the empty or failure text. The feed starts inWithBuildup(asTemplate.Binddoes), is one ordered subscription, and a failure is logged and shown.FeedBinding.BindMarkdown(markdownFeed, id, loading, failed)— the same for text only the hub can compute.Tabs
GetMeshNodeStream().Take(1)withWithValue(statement)baked inAdmin/Privacy's markdown, as the referenceMarkdownEditLayoutArea. The slot still waits forEnsureExists— a create-on-absent write precondition (binding to an absent node trips the storm breaker), not a read of valuesBehaviour note for review: per-row action buttons became select a row, then act — a button inside a bound row cannot say which row it is (platform gap, written up in DataBinding.md), while
DataGridCellClickcarries the row. Nothing actionable selected ⇒ the action refuses and says so.Ratchet: seven memex lines removed,
TotalBudget69 → 62.Evidence
FedControlsAreTemplatesTest(Layout.Test): feeds held silent — grid and markdown arrive at once, pointer-bound; first rows, a later value, an empty set and a failure all reach the same slot, read throughDataBind(the Blazor view's own path, which matters: a nested/data/x/loadingpointer read throughGetDataStreamnever deliveredfalse, so loading/empty live in top-level slots). Negative control: the oldfeed.Select(=> control)shape fails the first wait for both.WhatsNewTabIsATemplateTest(Memex.Portal.Shared.Test): end to end through a node hub; an entry created while the tab is open reaches the bound slot. Negative control: the pre-conversion tab never shows a pointer-bound markdown.MeshWeaver.Layout.Test(543),LocalizationTest, Documentation guards incl.LayoutAreaDataBakeRatchetGuardandSubscribeErrorArmRatchetGuard, memex guards/census and the existing settings-tab tests green locally; Release-warnaserrorclean for Layout, Messaging.Hub, Memex.Portal.Shared, Documentation and the three test projects.Deploy
Compiled portal code; nothing in-mesh. No recycle beyond the roll.
Pairs-with: none — no public type or member removed (removed members were private).
Implementers: none — no interface member added.
Mirror-sync: run
npm run sync:i18n -- --ref <merged core sha>in MeshWeaver.Plugins after this merges (addsinbox.*,invitations.*,serviceIdentities.tokensHeading|column.token|selectToken,instances.error.listFailed,privacy.loadFailed|placeholder,ui.updateStatusUnavailable).🤖 Generated with Claude Code