From efd4c67cc0957c2a938e606b24235aeecad6a868 Mon Sep 17 00:00:00 2001 From: Loretta Date: Fri, 17 Jul 2026 06:31:41 +0200 Subject: [PATCH] Fix filter panel restore race; doc v2 plan & logging Refactored MgGridFilterPanelTagBox to robustly heal filter criteria vs. async lookup data race, using level-triggered snapshot detection and reload. Added diagnostic logging. Updated MGGRID docs to explain the new logic, rationale, and planned v2 layout envelope solution. Minor unrelated shell command tweaks in settings.local.json. --- .../Grids/MgGridFilterPanelTagBox.cs | 159 ++++++++++++++---- .../docs/MGGRID/MGGRID_ISSUES.md | 9 +- .../docs/MGGRID/MGGRID_PARAMETERS.md | 8 +- .../docs/MGGRID/MGGRID_TODO.md | 34 +++- 4 files changed, 169 insertions(+), 41 deletions(-) diff --git a/AyCode.Blazor.Components/Components/Grids/MgGridFilterPanelTagBox.cs b/AyCode.Blazor.Components/Components/Grids/MgGridFilterPanelTagBox.cs index bc8329b..4d9eebe 100644 --- a/AyCode.Blazor.Components/Components/Grids/MgGridFilterPanelTagBox.cs +++ b/AyCode.Blazor.Components/Components/Grids/MgGridFilterPanelTagBox.cs @@ -45,43 +45,111 @@ public class MgGridFilterPanelTagBox : MgTagBox, I InheritSizeModeFromGrid(); - // Keep the selection in step with our field's active criteria on EVERY parameter pass. The editor - // resolves its tags against the CURRENT Data, and the criteria (layout restore via JS interop) and the - // lookup Data (async load) arrive in arbitrary order — one-shot "did X change" gates proved to get - // consumed in the wrong interleaving and strand an applied-but-invisible selection. This form is - // idempotent: it re-asserts Values (fresh instance => tags re-resolve) whenever the Data instance - // changed or the displayed selection drifted from the criteria, and no-ops otherwise. - if (Grid is not null && !string.IsNullOrWhiteSpace(FieldName) - && Grid.GetFilterPanelCriteria(FieldName) is { } activeCriteria - && TryExtractInValues(activeCriteria, out var criteriaValues)) - { - // Content signature, not just reference: consumers routinely bind a pre-created EMPTY collection and - // fill the SAME instance later (AcObservableCollection.Replace) — the reference never changes, only - // the count does (0 => N). Count() is O(1) for ICollection-backed lookups. - var dataCount = Data?.Count() ?? -1; - var sameInstanceRefilled = ReferenceEquals(Data, _lastSyncedData) && dataCount != _lastSyncedDataCount; - var dataChanged = !ReferenceEquals(Data, _lastSyncedData) || dataCount != _lastSyncedDataCount; - var selectionDrifted = Values is null || !Values.SequenceEqual(criteriaValues); - - if (dataChanged || selectionDrifted) - { - _lastSyncedData = Data; - _lastSyncedDataCount = dataCount; - - // DevExpress snapshots the editor's item list and detects Data changes BY REFERENCE — an in-place - // fill is invisible to it, so the re-asserted Values could never resolve into tags (re-rendering - // does not help). Reload() (DxListEditorBase) forces the editor to re-read its Data source. - if (sameInstanceRefilled) Reload(); - - SetValuesCore(criteriaValues); - } - } + TrackEditorSnapshot(); + Diag("OnParametersSet"); + SyncSelectionToCriteria(); } - // Last Data instance + count a re-assert ran against — only consumed when an assignment actually happens - // (see OnParametersSet; a change while there is no active criteria must NOT count as handled). - private IEnumerable? _lastSyncedData; - private int _lastSyncedDataCount = -1; + // ======================= TEMP DIAGNOSTIC — REMOVE after the restore-race investigation ======================= + // Purpose: prove/disprove that DevExpress raises ValuesChanged with an EMPTY list around Reload() (dropping + // not-yet-resolvable tags), which OnSelectionChanged would misread as a user edit and clear the criteria. + // Blazor WASM: Console.WriteLine lands in the browser console. Filter the console by "[MgFP". + + private static string Fmt(IEnumerable? values) => values is null ? "null" : $"[{string.Join(",", values)}]"; + + private void Diag(string message) + { + string criteria; + try + { + criteria = Grid is not null && !string.IsNullOrWhiteSpace(FieldName) + && Grid.GetFilterPanelCriteria(FieldName) is { } activeCriteria + ? activeCriteria.ToString() + : "null"; + } + catch (Exception ex) + { + criteria = $"ERR:{ex.GetType().Name}"; + } + + var dataCount = Data is null ? -1 : Data.Count(); + + Console.WriteLine($"[MgFP:{FieldName}] {message} | dataCount={dataCount} snapWasEmpty={_snapshotTakenWhileDataEmpty} " + + $"values={Fmt(Values)} criteria={criteria}"); + } + // ===================== /TEMP DIAGNOSTIC ===================================================================== + + // ---- Editor-snapshot state ----------------------------------------------------------------------------- + // A restore has TWO inputs that arrive in arbitrary order: the criteria (async layout restore via JS interop) + // and the lookup Data (async load — routinely filled IN PLACE via AcObservableCollection.Replace on a + // pre-created empty instance). DevExpress snapshots the editor's item list and detects Data changes BY + // REFERENCE ONLY, so an in-place fill leaves the editor holding an EMPTY list and no asserted Values can ever + // resolve into tags. + // This state is LEVEL-triggered by design: it records the standing fact "this editor's snapshot was taken + // while Data was empty" instead of trying to catch the 0->N edge. Edge/one-shot gates were tried twice and + // both systematically stranded the LAST-filled lookup — its arming pass never happens (see ADR 0004). + + private IEnumerable? _snapshotData; + private bool _snapshotTakenWhileDataEmpty; + private bool _reassertAfterReloadPending; + + private bool IsDataEmpty => Data is null || !Data.Any(); + + /// + /// Records whether the editor's current item snapshot was taken against an EMPTY Data. Runs on every + /// parameter pass REGARDLESS of criteria presence — arming must not depend on the criteria arriving first; + /// that ordering assumption is exactly what stranded the last-filled lookup. + /// + private void TrackEditorSnapshot() + { + if (!ReferenceEquals(Data, _snapshotData)) + { + // A NEW instance is detected by DevExpress, which re-reads the list itself => the editor's snapshot + // matches whatever this instance currently holds. + _snapshotData = Data; + _snapshotTakenWhileDataEmpty = IsDataEmpty; + return; + } + + // Same instance, still empty => the snapshot is (still) an empty list, and a later in-place fill will be + // invisible to DevExpress. Same instance, non-empty => leave the flag alone: it answers "was the snapshot + // taken before the fill?", which only a heal may clear. + if (IsDataEmpty) _snapshotTakenWhileDataEmpty = true; + } + + /// + /// Forces the editor to re-read its Data source when its snapshot is known-stale (taken while empty) + /// and Data has since been filled in place. Returns true when a reload was issued, and queues ONE + /// deferred re-assert: DevExpress documents no settle guarantee for Reload() + /// (DxListEditorBase.Reload is a bare void), so a same-pass Values assignment could still + /// race the item list. + /// + private bool HealEditorSnapshotIfStale() + { + if (!_snapshotTakenWhileDataEmpty || IsDataEmpty) return false; + + _snapshotTakenWhileDataEmpty = false; + Reload(); + _reassertAfterReloadPending = true; + + return true; + } + + /// + /// Brings the displayed selection in line with our field's active criteria: heals a stale editor snapshot + /// first, then asserts the values when the snapshot was just healed or the selection drifted. + /// + private void SyncSelectionToCriteria() + { + if (Grid is null || string.IsNullOrWhiteSpace(FieldName)) return; + if (Grid.GetFilterPanelCriteria(FieldName) is not { } criteria) return; + if (!TryExtractInValues(criteria, out var criteriaValues)) return; + + var healed = HealEditorSnapshotIfStale(); + var drifted = Values is null || !Values.SequenceEqual(criteriaValues); + + if (healed || drifted) SetValuesCore(criteriaValues); + } /// /// Defaults SizeMode to the grid's (via the cascade) so the filter panel matches the @@ -114,6 +182,22 @@ public class MgGridFilterPanelTagBox : MgTagBox, I // registration weakly and reconciles this component immediately — restoring a persisted panel filter // into the UI, or clearing it while the panel is hidden. See IMgGridFilterPanelComponent / ADR 0004. if (firstRender) Grid?.RegisterFilterPanelComponent(this); + + // Deferred re-assert after a Reload(): the reload-triggered render has completed by now, so the editor's + // item list is settled even though Reload() itself gives no such guarantee. Flag-gated and idempotent — + // the re-assert's own render finds the flag cleared, so this cannot loop. + if (_reassertAfterReloadPending) + { + _reassertAfterReloadPending = false; + + if (Grid is not null && !string.IsNullOrWhiteSpace(FieldName) + && Grid.GetFilterPanelCriteria(FieldName) is { } criteria + && TryExtractInValues(criteria, out var values)) + { + SetValuesCore(values); // fresh instance => the editor re-resolves its tags + StateHasChanged(); + } + } } private void OnSelectionChanged(IEnumerable? values) @@ -147,6 +231,11 @@ public class MgGridFilterPanelTagBox : MgTagBox, I if (criteria is not null && TryExtractInValues(criteria, out var values)) { + // The grid may be telling us about a criteria whose lookup data has only just arrived — e.g. the + // consumer calling RefreshFilterPanel() right after an in-place fill. Without healing the editor's + // (empty) snapshot first, the assert below could never resolve into tags: this is what made + // RefreshFilterPanel fall short of its documented contract. + HealEditorSnapshotIfStale(); SetValuesCore(values); } else diff --git a/AyCode.Blazor.Components/docs/MGGRID/MGGRID_ISSUES.md b/AyCode.Blazor.Components/docs/MGGRID/MGGRID_ISSUES.md index 64ecfbb..f5feddd 100644 --- a/AyCode.Blazor.Components/docs/MGGRID/MGGRID_ISSUES.md +++ b/AyCode.Blazor.Components/docs/MGGRID/MGGRID_ISSUES.md @@ -84,12 +84,13 @@ record which fields were panel-owned. Reset layout (toolbar) on the affected client, or re-save the layout from a client that has the panel. ### Proposed fix -Wrap the persisted layout in an MgGrid envelope (`{ dxLayout, quickFilterFields[] }`) so the loader can clear -quick-filter fields even with no registered controls. Deliberately deferred until the scenario is actually hit -(YAGNI); referenced from ADR 0004 (AyCode.Core repo). +Wrap the persisted layout in an MgGrid envelope (`MgGridLayoutSave { Version, GridLayoutJson, +FilterPanelLayoutJson }`) so panel state never rides inside the DevExpress layout: an entry no component claims +simply never reaches the grid — this issue is then closed **by construction**, no clearing logic needed. Design in +ADR 0004 (AyCode.Core repo) → "Planned v2". ### Related TODO -None yet. +`MGGRID_TODO.md#acblazor-grid-t-w8q3` (the envelope + pull-model restore — closes this issue). ## Issue entry template diff --git a/AyCode.Blazor.Components/docs/MGGRID/MGGRID_PARAMETERS.md b/AyCode.Blazor.Components/docs/MGGRID/MGGRID_PARAMETERS.md index cd78c76..7d79da3 100644 --- a/AyCode.Blazor.Components/docs/MGGRID/MGGRID_PARAMETERS.md +++ b/AyCode.Blazor.Components/docs/MGGRID/MGGRID_PARAMETERS.md @@ -80,12 +80,18 @@ A nullable FK column (`int?`) behaves the same — rows whose value is `null` ma **Persistence & visibility.** The panel's criteria is part of the grid's persisted layout (`GridPersistentLayout.FilterCriteria` — the AutoSave tier and the server-roamed UserSave tier alike; see `MGGRID_LAYOUT.md`). Components register with the grid (`IMgGridFilterPanelComponent`, weakly held; `MgGridFilterPanelTagBox` does this automatically) and the grid **reconciles** them after layout restore (incl. on another machine), user-layout load, layout reset, and panel toggle: -- panel **visible** → each component re-reads its field's criteria and re-selects (a restored panel filter is visible again); the restore (JS interop) and an async lookup `Data` arrive in arbitrary order — the tag box re-asserts its selection idempotently on every parameter pass while an owned criteria is active, tracking both the `Data` **instance and its item count** (an in-place fill — `AcObservableCollection.Replace` on a pre-created empty collection — is detected too, and triggers the editor's `Reload()`: DevExpress snapshots its item list and detects `Data` changes by reference, so an in-place fill would otherwise never resolve into tags), so tags resolve as soon as both exist; consumers can additionally force a re-run via `IMgGridBase.RefreshFilterPanel()` after a late lookup load; +- panel **visible** → each component re-reads its field's criteria and re-selects (a restored panel filter is visible again). See **Late lookup data** below for why this is more than a re-render; - panel **hidden** (toolbar toggle or `ShowFilterPanel="false"`) → the components' **own** criteria are **cleared** — a panel filter never filters invisibly. Hiding the panel therefore deliberately discards the current selection (filter-row values survive, per Ownership above); - a per-field criteria the component does not own is left untouched — the component just shows no selection. The panel content is always rendered (CSS-hidden when toggled off) so the components stay registered. Known edge — a layout with panel criteria loaded where the grid has no `FilterPanel` at all: `MGGRID_ISSUES.md` → `ACBLAZOR-GRID-I-D4T7`. +**Late lookup data (why restore is not just a re-render).** A restore has two inputs arriving in **arbitrary order**: the criteria (async layout restore) and the lookup `Data` (async load). DevExpress **snapshots the editor's item list and detects `Data` changes by REFERENCE only** — so the common consumer shape of binding a **pre-created empty** collection that is later filled **in place** (`AcObservableCollection.Replace`, unchanged reference) leaves the editor holding an empty list, and asserted `Values` can never resolve into tags no matter how often the component re-renders. + +`MgGridFilterPanelTagBox` therefore tracks a **level-triggered** fact — *"this editor's snapshot was taken while `Data` was empty"* — armed on **every** parameter pass regardless of criteria presence, and cleared only once it has healed: `DxListEditorBase.Reload()` (forcing a re-read of `Data`) plus a fresh-instance `Values` assert, run from **both** the parameter pass **and** `SyncFromGrid`, followed by one deferred post-render re-assert (`Reload()` carries no documented settle guarantee). Consumers may additionally force a reconcile via `IMgGridBase.RefreshFilterPanel()` after a late lookup load. + +> **Why level-triggered:** two earlier **edge-triggered** designs (detect the `Data` reference change; then reference **or** item-count change) each looked correct and each shipped a bug: an edge detector needs an *arming* pass that records the pre-fill state, so the **last-filled** lookup on a page — whose fill lands after its final parameter pass — was systematically stranded (rows filtered, `Clear` enabled, chips empty). Do not reintroduce an edge gate here. The v2 layout envelope removes the need for this heal entirely (see ADR 0004 → planned v2). + Implementation note: imperative parameter writes inside DevExpress editors (`Values`, `SizeMode`) must be wrapped in `BeginUpdate()`/`EndUpdate()` (the parameter tracker throws otherwise) — `MgGridFilterPanelTagBox` handles this internally. Design + the server-side forward path (`FilterMode.Server`): AyCode.Core repo `docs/adr/0004-signalr-datasource-server-side-filtering.md`. diff --git a/AyCode.Blazor.Components/docs/MGGRID/MGGRID_TODO.md b/AyCode.Blazor.Components/docs/MGGRID/MGGRID_TODO.md index f299d49..e0e84b8 100644 --- a/AyCode.Blazor.Components/docs/MGGRID/MGGRID_TODO.md +++ b/AyCode.Blazor.Components/docs/MGGRID/MGGRID_TODO.md @@ -2,13 +2,45 @@ For known issues / bugs see `MGGRID_ISSUES.md`. -> **Server-side grid filtering — design in ADR 0004.** MgGrid `FilterMode` (Local/Server), the criteria-string forwarding, and the `DxTagBox` quick-filter template extension are captured in `AyCode.Core/docs/adr/0004-signalr-datasource-server-side-filtering.md` (in AyCode.Core repo, Status: Proposed). Not yet broken into per-step entries — reference the ADR when implementation begins. +> **Server-side grid filtering — design in ADR 0004** (`AyCode.Core/docs/adr/0004-signalr-datasource-server-side-filtering.md`, in AyCode.Core repo). +> +> **Landed (2026-07-16/17), local mode:** the declarative filter panel — `FilterPanel` slot, `MgGridFilterPanelTagBox` (+ `IMgGridFilterPanelComponent`), per-field `IN` criteria via `SetFilterPanelCriteria` → DevExpress `SetFieldFilterCriteria`, toolbar Show/Hide toggle, built-in Clear button, layout persistence + visibility reconcile. See `MGGRID_PARAMETERS.md` → Filter Panel. +> +> **Still open (not yet broken into per-step entries — reference the ADR when implementation begins):** `FilterMode` (Local/Server) + criteria forwarding into `SignalParams.FilterCriteria`; `[FilterCriteriaParam]` hub injection + registration-time convention validation + mismatch warning (AyCode.Services / .Server); the filter-service (whitelist, parse/length hard-reject); `[NotGridFilterable]` (AyCode.Core, deferred); v2 paging. Plus `ACBLAZOR-GRID-T-W8Q3` below (the layout envelope), which the ADR designates as the successor to the current restore heal. ## Priority legend - **P0** blocker · **P1** important · **P2** nice-to-have · **P3** idea --- +## ACBLAZOR-GRID-T-W8Q3: `MgGridLayoutSave` envelope + pull-model filter-panel restore +**Priority:** P1 · **Type:** Refactor (framework-first) · **Origin:** 2026-07-17 multi-agent restore-race audit · **Related:** `MGGRID_ISSUES.md#acblazor-grid-i-d4t7`; design in `AyCode.Core/docs/adr/0004-signalr-datasource-server-side-filtering.md` → "Planned v2" + +Filter-panel criteria currently persists **inside** the DevExpress layout (`GridPersistentLayout.FilterCriteria`), so on restore the criteria reaches the grid **before** the panel can display it. That forces the framework to *detect* when the editor is able to show it — the source of two shipped edge-gate bugs and of the current level-triggered snapshot heal (`MgGridFilterPanelTagBox`). + +Replace with an envelope that gives each piece of state a single owner: + +```csharp +public class MgGridLayoutSave +{ + public int Version { get; set; } = 1; // 0 = legacy raw GridPersistentLayout JSON (no envelope) + public string? GridLayoutJson { get; set; } + public string? FilterPanelLayoutJson { get; set; } +} +``` + +- **Save:** for each registered component whose `OwnsCriteria` matches its field slot, REMOVE that criteria from the DX layout and serialize it into `FilterPanelLayoutJson` as `field → invariant-culture value strings`. +- **Restore** (all three funnels — `Grid_LayoutAutoLoading`, `LoadUserLayoutAsync`, `ResetLayoutAsync`): apply the panel-free DX layout; **park** the panel map; hand an entry to its component only when it is ready (registered AND `Data` non-empty), which then sets criteria + selection **atomically**. +- **Version:** `0` (absent property) = legacy raw layout → one-time migration extracting owned-shaped `IN` criteria (the only place shape-sniffing survives). Bump only for **semantic** changes — `System.Text.Json` ignores unknown properties, so additive sections need no bump. +- Never-claimed entries stay parked for the session and are **re-persisted** on save (a temporarily absent panel must not silently destroy a saved filter). + +### Acceptance criteria +- A restored panel filter never filters rows while its UI cannot show it (closes the rows-filtered/chips-empty window). +- `ACBLAZOR-GRID-I-D4T7` closed by construction (an ownerless entry never reaches the grid). +- `Reload()`, the snapshot-heal state, the pending-reconcile flag and `IMgGridBase.RefreshFilterPanel()` all become deletable; `RefreshFilterPanel` kept one release as a deprecated no-op shim, consumer call sites removed (e.g. `GridShippingDocument.ReloadDataFromDb`). +- Legacy (v0) layouts in localStorage and on the server still load. +- Interleaving tests: fill-order swap, criteria-before-data, data-before-criteria, no-panel restore. + ## ACBLAZOR-GRID-T-V4P7: Generic ID generation in `MgGridBase.SetNewId` **Priority:** P2 · **Type:** Refactor (framework-first) · **Origin:** 2026-04-24 in-code TODO audit · **Area:** `Components/Grids/MgGridBase.cs:460`