Skip to content

Flip LoggingMiddleware default to type-name-only and tighten selected-events signatures#514

Merged
jschick04 merged 1 commit into
mainfrom
jschick/logging-middleware-safe-default
May 4, 2026
Merged

Flip LoggingMiddleware default to type-name-only and tighten selected-events signatures#514
jschick04 merged 1 commit into
mainfrom
jschick/logging-middleware-safe-default

Conversation

@jschick04
Copy link
Copy Markdown
Collaborator

Summary

Flips the Fluxor LoggingMiddleware policy from "default = JSON-serialize unknown actions, allowlist = type-only" to "default = type-name only (safe), opt-in = rich diagnostics", and tightens the two EventLogAction selection records so the middleware can read .Count (property) instead of .Count() (LINQ).

Why

The original switch had a default: branch that JSON-serialized the entire action payload to debug.log on every dispatch, with a per-case allowlist for type-only logging. Four high-volume actions silently fell through to the default JSON path:

  • EventTableAction.AppendTableEvents (carries IReadOnlyList<DisplayEventModel>)
  • EventTableAction.AppendTableEventsBatch (carries IDictionary<int, IReadOnlyCollection<DisplayEventModel>>)
  • EventLogAction.LoadEventsPartial (carries IReadOnlyList<DisplayEventModel>)
  • EventLogAction.SetSelectedEvents (carries the user's full selection)

Result: every dispatch of these actions wrote a multi-kilobyte JSON blob to disk on the dispatch hot path. User caught it in the wild via Action: EventLogExpert.UI.Store.EventTable.EventTableAction+AppendTableEvents log lines that should never have included payload at all.

Changes

LoggingMiddleware.cs (-43 / +19):

  • default: branch now logs only action.GetType().
  • Removed 21-case type-name-only allowlist (those types now get the same output via the new safe default — no behavior change for them).
  • Removed try/catch and _serializerOptions field (no longer reachable on the hot path; only StatusBarAction.SetEventsLoading still serializes and its payload is small/bounded — guid + 2 ints).
  • Added explicit count-summary cases for the 4 large-payload actions.
  • Renamed addEventsActionaddEventAction (action is AddEvent, singular).

EventLogAction.cs (+2 / -2): Tightened SelectEvents.SelectedEvents and SetSelectedEvents.SelectedEvents from IEnumerable<DisplayEventModel> to IReadOnlyCollection<DisplayEventModel>. Every call site already passes List<T> or T[] (verified via repo-wide grep); the broader type was fiction. Tightening unlocks .Count (property, O(1)) on the dispatch hot path.

EventLogStoreTests.cs (+1 / -1): .Count().Count on the tightened signature for sibling-consistency with the other 5 .Count sites in the file.

LoggerUtils.cs (new): RecordingTraceLogger test fake at src/EventLogExpert.UI.Tests/TestUtils/LoggerUtils.cs. NSubstitute can't cleanly capture interpolated string handler structs (ITraceLogger.Debug([InterpolatedStringHandlerArgument(""")] DebugLogHandler handler)); the fake calls handler.ToStringAndClear() synchronously and stores per-level List<string> for assertions.

Verification

  • Full solution build: 0 warnings, 0 errors.
  • All 1791 tests pass (113 Components + 19 EventDbTool + 628 Eventing + 1031 UI).
  • Multi-model code review (Claude Opus + GPT-5.5): both clean. Opus's one finding (.Count().Count outlier in EventLogStoreTests.cs:190) was applied.

Copilot AI review requested due to automatic review settings May 4, 2026 20:00
@jschick04 jschick04 requested a review from a team as a code owner May 4, 2026 20:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR changes the UI logging middleware to default to type-name-only logging, adds explicit summary logging for several high-volume actions, and narrows two event-selection action payload types to IReadOnlyCollection<T> so count access is O(1).

Changes:

  • Simplified LoggingMiddleware so most actions now log only their type name by default, with explicit count-based summaries for large event payloads.
  • Tightened EventLogAction.SelectEvents and SetSelectedEvents from IEnumerable<DisplayEventModel> to IReadOnlyCollection<DisplayEventModel>.
  • Added a test logger utility and updated one test assertion to use .Count instead of .Count().

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
src/EventLogExpert.UI/Store/LoggingMiddleware.cs Reworks action logging behavior and adds summary logging for large event collections.
src/EventLogExpert.UI/Store/EventLog/EventLogAction.cs Narrows public action record parameter types for selected-event collections.
src/EventLogExpert.UI.Tests/TestUtils/LoggerUtils.cs Adds a test fake logger that records rendered interpolated log messages.
src/EventLogExpert.UI.Tests/Store/EventLog/EventLogStoreTests.cs Updates a test to match the tightened collection type usage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/EventLogExpert.UI/Store/EventLog/EventLogAction.cs
Comment thread src/EventLogExpert.UI/Store/EventLog/EventLogAction.cs
Comment thread src/EventLogExpert.UI/Store/LoggingMiddleware.cs
Comment thread src/EventLogExpert.UI/Store/LoggingMiddleware.cs
Comment thread src/EventLogExpert.UI/Store/LoggingMiddleware.cs
@jschick04 jschick04 merged commit a5bba9c into main May 4, 2026
10 checks passed
@jschick04 jschick04 deleted the jschick/logging-middleware-safe-default branch May 4, 2026 21:30
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.

3 participants