Skip to content

refactor(core)!: share diagnostics warning log and per-form latch across adapters (#398) - #407

Merged
phmatray merged 6 commits into
devfrom
feat/398-share-diagnostics-log-and-latch
Sep 23, 2026
Merged

phmatray merged 6 commits into
devfrom
feat/398-share-diagnostics-log-and-latch

Conversation

@phmatray

@phmatray phmatray commented Sep 23, 2026 •

Copy link
Copy Markdown
Owner

Implements #398.

Closes #398.

Executing the implementation plan task-by-task; the checklist below — and the plan on the issue — are
ticked as each task lands.

Plan

  • Task 1: Move DiagnosticLog and the per-form latch into core
  • Task 2: Route FluentUI's custom-template warning through the moved types

Review (Standards / Spec / Verification, run per implement-issue Step 7)

Standards — one real finding: a stale doc comment in
FormCraft.ForFluentUI.UnitTests/Components/CustomTemplateTests.cs claiming the adapter has "no
shared diagnostics infrastructure" (now false, since this PR gives it one) and naming the renamed
DiagnosticLog type. Fixed.

Spec — flagged that FormCraftComponent.razor.cs (FluentUI) now emits its custom-template
warning under a new fixed category, "FormCraft.ForFluentUI.CustomTemplateField", in place of the
prior per-TModel ILogger<FormCraftComponent<TModel>>>() category, and read the issue's Spec text
("FluentUI... keeps its own... category string for the unresolved-custom-template-field diagnostic")
as forbidding that. Verdict: disagree, kept as implemented. The issue's own Implementation
Plan
(Task 2, Step 3) explicitly directs adding this exact constant, "matching MudBlazor's call
shape exactly" — MudBlazor's identical diagnostic already uses a named category
(FormCraft.ForMudBlazor.CustomTemplateField), so FluentUI's prior per-TModel category was itself
the inconsistency this issue exists to remove, not a fact to preserve. The Spec text's assumption
that FluentUI "already has" a dedicated category string doesn't match the pre-PR code. Also flagged
(and independently found by the Verification pass): the issue's acceptance criterion 4's grep
pattern was already non-clean before this PR touched anything, because of the unrelated, pre-existing
SecurityEnforcer (#321/#402) logger resolution at the same file — the criterion's grep is
over-broad, not something this PR's diff could ever satisfy literally.

Verification — one real gap: the new category string had no test asserting its value (only
message text was checked). Fixed — CustomTemplateTests.cs's local CapturingLoggerProvider now
tracks category like the core one does, and the regression test asserts
entries[0].Category.ShouldBe("FormCraft.ForFluentUI.CustomTemplateField"). Also caught: an
Entries property on FormCraft.ForMudBlazor.UnitTests/TestSupport/CapturingLoggerProvider.cs left
with zero callers once DiagnosticLogTests moved to core — removed.

Notes for the owner

  • Source-compat break, by design of the issue. FormDiagnosticScope was public in
    FormCraft.ForMudBlazor (cascaded as a public parameter on MudBlazorFieldComponentBase), not
    internal as the issue's Spec assumed. Moving it to FormCraft.Diagnostics without a forwarding
    shim (per the issue's own explicit instruction — "not kept as a compatibility forwarder") is a
    source break for any third-party code that referenced FormCraft.ForMudBlazor.FormDiagnosticScope
    directly. This is a preview series (3.1.1-preview.*) and the demo apps build clean, but the PR
    title (refactor(core): ...) won't flag this to release-please the way a ! would. Flagging here
    for a call on whether to add one.

Fixed along the way

  • Stale doc comment (Standards finding above).
  • Verification gap: category assertion on FluentUI's custom-template diagnostic (Verification finding above).
  • Dead Entries property on FormCraft.ForMudBlazor.UnitTests's CapturingLoggerProvider, orphaned by moving DiagnosticLogTests to core.

Follow-ups

BREAKING CHANGE: FormDiagnosticScope and DiagnosticLog move from FormCraft.ForMudBlazor to FormCraft.Diagnostics (core). Code that references them by the old namespace must update its using directive. No forwarding shim is provided.

BEGIN_COMMIT_OVERRIDE
refactor(core)!: share diagnostics warning log and per-form latch across adapters (#398)

BREAKING CHANGE: FormDiagnosticScope and DiagnosticLog moved from FormCraft.ForMudBlazor to FormCraft.Diagnostics (core). Code that references them by the old namespace must update its using directive. No forwarding shim is provided.
END_COMMIT_OVERRIDE

@phmatray
phmatray marked this pull request as ready for review September 23, 2026 23:30
@phmatray phmatray changed the title refactor(core): share diagnostics warning log and per-form latch across adapters (#398) refactor(core)!: share diagnostics warning log and per-form latch across adapters (#398) Sep 23, 2026
@phmatray
phmatray merged commit c0044cf into dev Sep 23, 2026
5 checks passed
@phmatray
phmatray deleted the feat/398-share-diagnostics-log-and-latch branch September 23, 2026 23:36
phmatray added a commit that referenced this pull request Sep 23, 2026
… diagnostics move

#407 (merged into dev while this branch was in flight) renamed DiagnosticLog to
FormDiagnosticLog and moved it into FormCraft.Diagnostics. The merge picked up
the rename on the read side (GetCustomTemplateValue) automatically but left the
write side (UpdateFieldValue), added by this branch after #407 branched, on the
old name.
phmatray added a commit that referenced this pull request Sep 23, 2026
… custom templates (#413)

* feat(core): add FieldValueSetterCache mirroring the value-getter cache

Compiles a write-back Action<TModel, object?> from a field's ValueExpression,
unwrapping the Convert node FieldConfigurationWrapper adds and assigning
through the resulting member chain at any depth. Coerces the incoming boxed
value to the target member's type first, the same Convert.ChangeType step
both adapters' UpdateFieldValue used to perform against a GetProperty(fieldName)
lookup that only ever resolved a top-level property.

Part of #396.

* fix(adapters): write back through the field's compiled expression for custom templates

UpdateFieldValue now takes the IFieldConfiguration<TModel, object> instead of a bare
field name and writes through FieldValueSetterCache<TModel>, replacing the
typeof(TModel).GetProperty(fieldName)/SetValue reflection that only ever resolved a
direct top-level property. A custom-template field bound to a nested path
(x => x.Nested.Value) now writes back exactly as reliably as it already reads (#330).

A write that fails (most commonly a null intermediate) is caught and reported once per
field through each adapter's existing #330 diagnostic latch instead of throwing out of
the event callback.

Closes #396

* fix(adapters): use the renamed FormDiagnosticLog after syncing #407's diagnostics move

#407 (merged into dev while this branch was in flight) renamed DiagnosticLog to
FormDiagnosticLog and moved it into FormCraft.Diagnostics. The merge picked up
the rename on the read side (GetCustomTemplateValue) automatically but left the
write side (UpdateFieldValue), added by this branch after #407 branched, on the
old name.

* fix(adapters): report the coerced post-write value on OnFieldChanged

UpdateFieldValue was reporting the caller's raw pre-coercion value through
OnFieldChanged, whereas the reflection-based code it replaced reported its own
Convert.ChangeType result. Read the value back through the same compiled
FieldValueGetterCache the render path already uses, which reflects the true
post-write state rather than re-deriving the conversion.

Also strengthens both adapters' "warn once" regression test: the custom
template body now reads context.Value, so the read path genuinely fires (and
warns) during the initial render before ValueChanged is invoked, so the
assertion that the write does not add a second warning actually exercises the
shared-latch dedup it claims to.

Found in code review of #396.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FluentUI hand-rolls its own diagnostics warning latch instead of sharing MudBlazor's DiagnosticLog

1 participant