Repository navigation
feat(plugincatalog): the coupon list's per-row Open button is back — a row-scoped action (reference conversion) - #5956
Conversation
…e carries its row
A bound row template (BindMany / ItemTemplateControl, a DataGrid TemplateColumnControl) exists
ONCE on the owner, so a button in it posted a ClickedEvent naming only the template's area and
its action could not tell which row was clicked. The data-binding batches had to downgrade
per-row buttons to "click the row, then act" or leave areas unconverted.
- RowContext (Pointer, Index, Value, Path; NodePath()) — the row AS THE CLIENT RENDERED IT.
- ClickedEvent.Row / BlurEvent.Row (init, non-breaking); the owner hands it to the action as
UiActionContext.Row. Never re-resolved by index: a list that changed between render and click
still acts on the clicked row.
- ctx.RowAs<T>() (Mesh.Contract, via ObjectAsExtensions.As) and ctx.RowPath().
- DataGridControl renders each template column's template into {grid}/Column{i}, so a cell's
control and its click action are found where the client's event names them (before, the
template lived only inline and a click reached the GRID's area).
- RowContext registered with the layout types.
Tests: RowScopedClickActionTest — N rows, row k acts on row k; a no-row negative control; the
list changing between render and click; a grid template column (red without the RenderSelf
change). Doc: GUI/DataBinding → "Row-scoped actions".
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…a row-scoped action The reference conversion for row-scoped actions. B3 part 3 made the coupon list a fed grid but had to drop the per-coupon Open button for "click the row" (a button in a bound row could not say which row it was). The Code column is now a TemplateColumnControl whose ONE button is labelled with the row's code (ContextProperty) and opens THAT coupon via ctx.RowAs<CouponRow>() — the row as clicked, so a list that refreshed since the render still opens the clicked coupon. - CouponGrid split from the feed (testable template half); OpenCouponButton / OpenCoupon. - CouponListOpensTheClickedCouponTest: row k opens coupon k; a refresh between render and click still opens the clicked coupon; a no-row click opens nothing (negative control). - DataBinding.md: the per-row-action bullet points at row-scoped actions; the "platform gap" paragraph now says it is closed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test Results (shard 1) 1 files - 2 1 suites - 2 5m 33s ⏱️ +20s Results for commit ecce743. ± Comparison against base commit 5978cbc. This pull request removes 1305 and adds 489 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Test Results (shard 4)676 tests 676 ✅ 37s ⏱️ Results for commit ecce743. ♻️ This comment has been updated with latest results. |
Test Results (shard 5)634 tests 634 ✅ 1m 20s ⏱️ Results for commit ecce743. ♻️ This comment has been updated with latest results. |
Test Results (shard 2)605 tests 605 ✅ 1m 50s ⏱️ Results for commit ecce743. ♻️ This comment has been updated with latest results. |
Test Results (shard 3) 2 files 2 suites 4m 27s ⏱️ Results for commit ecce743. ♻️ This comment has been updated with latest results. |
Test Results 9 files 9 suites 16m 40s ⏱️ Results for commit ecce743. ♻️ This comment has been updated with latest results. |
…coupon-list Conflicts: RowScopedClickActionTest (the cherry-picked #5955 commit; main's version kept), the PluginCatalog InternalsVisibleTo comment (now names both the catalog's and the coupon list's row-scoped actions) and DataBinding.md (this branch's per-row-action bullet, main's row-scoped catalog and Todo paragraphs). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…-scoped-coupon-list
|
Auto-merge was not armed. This pull request targets |
![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)
Converts the coupon admin list's per-row open from the grid-level `DataGridCellClick` handler to a row-scoped action: one `ButtonControl` in a `TemplateColumnControl`, labelled by the row's `code` via `ContextProperty`, whose `OpenCoupon` handler reads the row from the click context and navigates to `/{CouponsNamespace}/{code}`. The grid moves into an internal, testable `CouponGrid(host, rows)` template function; `MeshWeaver.Layout.Test` gains `CouponListOpensTheClickedCouponTest` (per-row opens, a refresh/stale-row open and a no-row negative control — all executing); DataBinding.md's per-row-action bullet now names this list as the row-scoped reference. Checked from the diff: the action's guard and navigation against the removed handler, the column wiring, the tests' assertions, and the csproj change (comment-only — the `MeshWeaver.Layout.Test` grant is unchanged context, though the PR description lists it as new). Not verifiable from the diff: the platform API this builds on (`RowAs<T>()`, `TemplateColumnControl` wiring, `ClickedEvent.Row`/`RowContext` — from the cherry-picked #5955 platform commit, whose Blazor half in MeshWeaver.Plugins#2656 must be live for a browser click to carry a row at all); `ToCamelCase()`'s declared nullability (what the `!` silences); whether `MeshWeaver.PluginCatalog` generates XML doc files (whether the malformed doc comment breaks the `-warnaserror` build or only garbles the member's doc); and the PR description's green claims (the item records no failed jobs, and no runs).
Findings: 2 blocking · 1 should-fix · 1 question · 1 nit
Internal review of 816de97a4f632fa3ef22020a82d19eabc77c2a2e — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| .WithView(Controls.Body(host.Localize("coupons.openHint")) | ||
| .WithStyle("color: var(--neutral-foreground-hint); margin-top: 8px;")); | ||
|
|
||
| /// <summary>The coupon grid over <paramref name="rows"/> — the template half of |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
The XML doc comment on the new `CouponGrid` method is malformed: its first line opens with a closing tag — `/// </summary>The coupon grid over …` — with no `<summary>` start tag anywhere, and the second line then ends with a second `</summary>`. Two end tags, no start; every other doc comment in this diff opens with `<summary>`. Wherever XML documentation files are generated this is CS1570 (badly formed XML), an error at the repo's `-warnaserror` bar; with doc generation off it still garbles the new member's documentation. The leading `</summary>` is a typo for `<summary>`.
There was a problem hiding this comment.
Checked on 816de97 and again on 8785476: the comment reads /// <summary>The coupon grid over <paramref name="rows"/> — the template half of / /// <see cref="CouponList"/>, separate from the feed so its row-scoped action is testable.</summary>. It has one start tag and one end tag. The leading </summary> in your view looks like the previous comment's closing line joined to this one in the patch. MeshWeaver.PluginCatalog builds Release -warnaserror with no CS1570.
| /// with the click (<see cref="OpenCoupon"/>). | ||
| /// </summary> | ||
| internal static ButtonControl OpenCouponButton() | ||
| => Controls.Button(new ContextProperty(nameof(CouponRow.Code).ToCamelCase()!)) |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
`OpenCouponButton()` appends a null-forgiving `!` — `nameof(CouponRow.Code).ToCamelCase()!` — when constructing the `ContextProperty`. The removed Code column passed the very same `ToCamelCase()` expression with no `!`, so the `!` is there to silence the nullability warning at the new non-null call site, which the repo's hard rule forbids (`!` used to silence a warning). If `ToCamelCase()` is `string?`-typed, the null belongs at the source — a non-nullable `ToCamelCase` or an explicit null case; if it is non-nullable, the `!` is dead syntax and still falls under the rule.
There was a problem hiding this comment.
Fixed in 8785476. The button now binds new ContextProperty(JsonNamingPolicy.CamelCase.ConvertName(nameof(CouponRow.Code))). That call returns a non-null string, so nothing is forgiven. ToCamelCase is string? -> string? (CamelCaseExtensions.cs:21), which is why the old call site needed the operator.
| - `LayoutTemplate.DeferredViews` / `Descendants` (`MeshWeaver.Layout`) is the shared form of the reference test's check: an empty list means the tree is a template. | ||
|
|
||
| - **A list with a per-row action (archive, revoke, rotate)** — a fed grid plus SELECTION: `WithClickAction` on the grid reads the clicked row from `DataGridCellClick` with `.As<TRow>(…)` and writes it to a `/data` slot (seeded with a "nothing selected" row whose label is the hint); a bound label shows the selection and the action buttons below the grid act on it, refusing — and saying so — when nothing actionable is selected. The settings tabs Inbox, Invitations and Service identities use it; a bound row whose button carries its row ([Row-scoped actions](#row-scoped-actions)) is the shape that replaces it. Keep the feed in its own function (it takes the host, builds no control) and the action in its own function, so the template function reads nothing. | ||
| - **A list with a per-row action (archive, revoke, rotate, open)** — a fed grid with a ROW-SCOPED button in a template column (see "Row-scoped actions" below): the button is declared once, and its click carries the row it was clicked in. The coupon admin list is the reference (`CouponAdminSettingsTab.CouponGrid`, pinned by `CouponListOpensTheClickedCouponTest`). The older shape, written before row-scoped actions existed, is a fed grid plus SELECTION: `WithClickAction` on the grid reads the clicked row from `DataGridCellClick` with `.As<TRow>(…)` and writes it to a `/data` slot (seeded with a "nothing selected" row whose label is the hint); a bound label shows the selection and the action buttons below the grid act on it, refusing — and saying so — when nothing actionable is selected. The settings tabs Inbox, Invitations and Service identities use it. Keep the feed in its own function (it takes the host, builds no control) and the action in its own function, so the template function reads nothing. |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
The rewritten bullet now presents the coupon admin list as THE reference for row-scoped actions, but the “Not convertible at the helper” paragraph just below — unchanged by this diff — still ends with: “The coupon and instance-grant admin lists, which open or act on a row, are fed grids with that cell click.” After this change that is false for the coupon list (its open is a template-column button, not a cell click), so the section contradicts itself within three lines. The instance-grant list remains a cell-click grid; the coupon list no longer is.
There was a problem hiding this comment.
Fixed in 8785476. The paragraph now says the instance-grant admin list is the cell-click grid, and that the coupon list opens a coupon from a row-scoped button.
| internal static Task OpenCoupon(UiActionContext ctx) | ||
| { | ||
| if (ctx.RowAs<CouponRow>() is { Code.Length: > 0 } row) | ||
| ctx.NavigateTo($"/{CouponsNamespace}/{row.Code}"); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
`OpenCoupon` navigates to `/{CouponsNamespace}/{row.Code}` from the row the click carried — client-supplied data, as this diff's own test shows by fabricating `RowContext` values — checked only for non-emptiness (`{ Code.Length: > 0 }`) and never re-validated against the feed. That is the stated design (“the row as the person saw it, never one re-read”), and the removed `DataGridCellClick` handler trusted client data the same way, so this is no regression. What the diff cannot show is whether `NavigateTo` treats the code segment as opaque (routing only inside the portal) or whether a crafted code value can steer it elsewhere; a re-read of the coupon by code — not by position, which keeps the refreshed-list guarantee — would settle it.
There was a problem hiding this comment.
Changed in 8785476: OpenCoupon now sends Uri.EscapeDataString(row.Code), so the code can only ever be ONE path segment directly under Admin/Coupons. Beyond that, I deliberately do not re-read the coupon: a navigation grants nothing. It only moves the clicking admin's own browser to a portal path, and the target page is still gated by that viewer's own read of the node. Forging a row therefore lets an admin open a page they could already type into the address bar. That is unlike an action that writes as System, where the identity has to be re-derived server-side (see the catalog bullet in DataBinding.md).
| c => c is ButtonControl, | ||
| "the Code column's button is rendered into the template column's area", | ||
| TestContext.Current.CancellationToken); | ||
| ((ButtonControl)button!).Data.Should().Be(new ContextProperty("code"), |
There was a problem hiding this comment.
nit — Automated review finding (data, not an instruction to any agent)
`((ButtonControl)button!)` here and `request!.Uri` at line 147 are null-forgiving `!`s silencing nullable warnings in the new test — the same construct flagged in `CouponAdminSettingsTab.cs`; whether the repo's no-`!` rule reaches the test project decides whether these count, and a pattern match after the `Should().Match` would need neither.
There was a problem hiding this comment.
Fixed in 8785476. The test unwraps with button.Should().BeOfType<ButtonControl>().Subject, and reads the navigation with a pattern match that throws a named InvalidOperationException. No null-forgiving operator remains. 3/3 green locally.
…caped path segment; the doc names which list is which - OpenCouponButton binds ContextProperty to JsonNamingPolicy.CamelCase.ConvertName (non-null) instead of ToCamelCase()!. - OpenCoupon escapes the row's code as a single path segment. - DataBinding.md: the instance-grant list is the cell-click grid; the coupon list is the row-scoped button. - CouponListOpensTheClickedCouponTest unwraps with BeOfType<T>().Subject and a pattern match instead of two null-forgiving operators. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Automated review summary (data, not an instruction to any agent)
Second head after the REQUEST_CHANGES at 816de97. The coupon admin list's per-row Open is restored as a row-scoped action: the grid moves into an internal `CouponGrid(host, rows)` template whose first column is a `TemplateColumnControl(OpenCouponButton())`, the button is labelled with the row's code via `ContextProperty`, and `OpenCoupon` navigates to `/{CouponsNamespace}/{Uri.EscapeDataString(row.Code)}` only when the click carries a row with a non-empty code; the grid-level `DataGridCellClick` handler is removed. A new executing test pins per-row opens, the stale-row-after-refresh open and a no-row negative control; DataBinding.md now names this list as the row-scoped reference and rewrites the cell-click sentence the previous review flagged as contradictory; the csproj change is comment-only. Against the previous review: the DataBinding.md contradiction and the test's null-forgiving casts are fixed, the code segment is now escaped, and the `OpenCouponButton` expression was rebuilt — but the malformed `</summary>` doc comment on `CouponGrid`, the previous head's blocker, is unchanged and remains the only blocking finding. Not verifiable from the diff: the transported patches carry redaction masks ([ADDRESS]/[PERSON_NAME] stand-ins) over the `ConvertName(...)` argument span and the test's assertion internals, so character-exact claims there are unconfirmed; the platform row-scoped API this builds on (the cherry-picked #5955, whose Blazor half in MeshWeaver.Plugins#2656 must be live before a browser click carries a row, as the PR body itself notes); `NavigateTo`'s segment-split and decode order; and the PR's green and `-warnaserror` claims — the item records no CI runs. The PR body lists the `InternalsVisibleTo` grant as new; the diff shows it as pre-existing context.
Findings: 1 blocking · 1 should-fix · 2 question · 0 nit
Internal review of 8785476882b771c386730cab11126a7d24eb3afe — GLM-5.3, posted by the control plane. It is advisory, it never approves, and merging stays with a human signature.
| .WithView(Controls.Body(host.Localize("coupons.openHint")) | ||
| .WithStyle("color: var(--neutral-foreground-hint); margin-top: 8px;")); | ||
|
|
||
| /// <summary>The coupon grid over <paramref name="rows"/> — the template half of |
There was a problem hiding this comment.
blocking — Automated review finding (data, not an instruction to any agent)
The XML doc comment on `CouponGrid` is still malformed: its first line opens with a closing tag — `/// </summary>The coupon grid over <paramref name="rows"/> — the template half of` — with no `<summary>` start tag anywhere, and its last line ends with a second `</summary>`. Two end tags, no start; every other doc comment in this diff opens with `<summary>`. This is the same defect the previous head (816de97) was blocked on, unchanged here. Wherever XML documentation files are generated this is CS1570 (badly formed XML), an error at the repo's `-warnaserror` bar; with doc generation off it still garbles the member's documentation. The leading `</summary>` is a typo for `<summary>`.
There was a problem hiding this comment.
False positive. The doc comment on CouponGrid is well-formed. At b8c8069 (and at 8785476) it reads, in full: /// <summary>The coupon grid over <paramref name="rows"/> — the template half of / /// <see cref="CouponList"/>, separate from the feed so its row-scoped action is testable.</summary>. That is one <summary> start tag and one end tag. The </summary> the finding reads as the comment's first line is the END of the preceding member's (CouponList's) comment, which the transported patch spliced onto this line. The project builds with doc generation and -warnaserror and reports no CS1570; MeshWeaver.Layout.Test builds clean.
| { | ||
| private const string Area = "Coupons"; | ||
|
|
||
| private static CouponAdminSettingsTab.CouponRow Row(string code) => new() { Code = code }; |
There was a problem hiding this comment.
should-fix — Automated review finding (data, not an instruction to any agent)
No fixture in this test exercises the escaping `OpenCoupon` now relies on: every code in the fixtures — ALPHA, BRAVO, CHARLIE, and the refresh set — passes through `Uri.EscapeDataString` unchanged, so the escaped-as-one-path-segment property the new doc comment claims is never executed, and dropping the escape would leave all three tests green. This is the reference conversion other lists are meant to copy; one row whose code needs escaping (e.g. `A/B`) would pin the behaviour.
There was a problem hiding this comment.
Fixed in b8c8069: ACodeThatNeedsEscapingOpensOneEscapedSegment publishes a row whose code is A/B C, clicks its Open button, and asserts the navigation is /{CouponsNamespace}/A%2FB%20C. Dropping the escape makes it fail. All 4 cases pass locally.
| /// with the click (<see cref="OpenCoupon"/>). | ||
| /// </summary> | ||
| internal static ButtonControl OpenCouponButton() | ||
| => Controls.Button(new ContextProperty(JsonNamingPolicy.CamelCase.ConvertName(nameof(CouponRow.Code)))) |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
The null-forgiving `!` that blocked the previous head on this expression is addressed by rebuilding it as `JsonNamingPolicy.CamelCase.ConvertName(nameof(CouponRow.Code))` — no `!` is visible in the transported patch — but the patch redacts the span from `nameof(CouponRow.` through `.WithAppearance`, so whether a trailing `!` survives inside that span is not verifiable from the diff. Related and also not decidable here: `JsonNamingPolicy.CamelCase` is `JsonNamingPolicy?`-annotated on current .NET, so under nullable enabled plus the repo's `-warnaserror` bar the bare receiver dereference would itself be CS8602-as-error; whether this compiles clean because the project targets an earlier TFM or runs with nullable off — and not because a `!` silences CS8602 — is the open point.
There was a problem hiding this comment.
No ! survives. The line is Controls.Button(new ContextProperty(JsonNamingPolicy.CamelCase.ConvertName(nameof(CouponRow.Code)))). On net10.0, JsonNamingPolicy.CamelCase is declared non-nullable (public static JsonNamingPolicy CamelCase { get; }), so no CS8602 arises. The project has nullable enabled and builds under -warnaserror with 0 warnings.
| internal static Task OpenCoupon(UiActionContext ctx) | ||
| { | ||
| if (ctx.RowAs<CouponRow>() is { Code.Length: > 0 } row) | ||
| ctx.NavigateTo($"/{CouponsNamespace}/{Uri.EscapeDataString(row.Code)}"); |
There was a problem hiding this comment.
question — Automated review finding (data, not an instruction to any agent)
`Uri.EscapeDataString(row.Code)` closes the previous head's traversal concern at the URI level — the escaped segment can contain no `/`, `?`, `#` or `%`. The new doc comment goes further and claims the code can only name a node directly under the coupons namespace; that stronger guarantee additionally requires the hub to split a navigation URI into segments before percent-decoding them — a decode-then-split router would turn an escaped `%2F` back into a separator inside the segment. `NavigateTo`'s parse order is not visible in this diff; that is the one part of the claim the escaping alone does not establish.
There was a problem hiding this comment.
Agreed: the escape alone does not establish the stronger claim. Fixed in b8c8069 by narrowing the doc comment to what the escape does prove: the code "cannot add a separator, a query or a fragment to the URI it builds". Opening the target is still gated by the viewer's own read of whatever node the URI resolves to, so a navigation grants nothing, whatever the router's decode order.
…ims only what escaping proves Review of 8785476 (should-fix 4174527889): no fixture exercised the escape — a code that needs it ('A/B C') now opens '/{ns}/A%2FB%20C'. Question 4174527896: the doc comment claimed the code can only name a node directly under the coupons namespace, which also depends on the router's decode order; it now says what the escape establishes — no separator, query or fragment is added. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…o feat/row-scoped-coupon-list
…s (CS0419) Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…o feat/row-scoped-coupon-list # Conflicts: # src/MeshWeaver.Documentation/Data/GUI/DataBinding.md
…n-list # Conflicts: # src/MeshWeaver.Documentation/Data/GUI/DataBinding.md
1 participant
Reference conversion for row-scoped actions (#5955): the coupon admin list's per-coupon Open button, which data-binding B3 part 3 (#5948) had to drop for "click the row", is back — as ONE button in a template column whose click carries its row.
What changed
CouponAdminSettingsTab.CouponGrid(the template half, split fromCouponRowsFeedso it is testable): the Code column is aTemplateColumnControlwhose button is labelled with the row's code (ContextProperty("code"), bound to the grid row) and runsOpenCoupon→ctx.RowAs<CouponRow>()→ navigate toAdmin/Coupons/{code}. The grid-level row click (DataGridCellClick) is gone. Same behaviour as the baked list's "one Open button per coupon", now in the fed grid.InternalsVisibleTo MeshWeaver.Layout.Teston PluginCatalog, for the pin below.Evidence
test/MeshWeaver.Layout.Test/CouponListOpensTheClickedCouponTest(3/3): the button renders at the template column's area labelled by the row's code; row k's button opens coupon k for every k; the coupons refresh between the render and the click (BRAVO deleted, another coupon in its slot) and the click still opens BRAVO; negative control — a click with no row opens nothing. Also green:RowScopedClickActionTest,FedControlsAreTemplatesTest,MeshWeaver.Documentation.Test(662/662, incl. the bake ratchet). Release-warnaserrorclean: MeshWeaver.PluginCatalog, MeshWeaver.Documentation, MeshWeaver.Layout.Test.Deploy
Compiled portal code; no recycle beyond the roll. The Blazor half (Systemorph/MeshWeaver.Plugins#2656) must be live for the browser to send the row; until then a click opens nothing (the negative-control behaviour), which is why this lands after both.
Pairs-with: none — no public type or member removed (
CouponGrid,OpenCouponButton,OpenCouponare new internals).Implementers: none — no interface member added.
Mirror-sync: none — no catalog key added or re-worded (reuses
ui.couponColumnCode,coupons.openHint).🤖 Generated with Claude Code