Skip to content

Add body content filtering to message and deadletter views#871

Open
simonhedlund-exs wants to merge 9 commits intopaolosalvatori:mainfrom
simonhedlund-exs:feature/body-search-filter
Open

Add body content filtering to message and deadletter views#871
simonhedlund-exs wants to merge 9 commits intopaolosalvatori:mainfrom
simonhedlund-exs:feature/body-search-filter

Conversation

@simonhedlund-exs
Copy link
Copy Markdown

@simonhedlund-exs simonhedlund-exs commented Apr 20, 2026

Implements #870

  • Add inline search boxes for filtering messages by body content and deadletter reason
  • Implement body content filtering with free text and JSON path matching
  • Add BodyFilterForm dialog for configuring body filters (free text, JSON path, SQL property filter)
  • Support case-sensitive toggle and auto-discovered JSON paths
  • Cache message bodies for filtering performance
  • Apply to both queue (HandleQueueControl) and subscription (HandleSubscriptionControl) views

- Add inline search boxes for filtering messages by body content and deadletter reason
- Implement body content filtering with free text and JSON path matching
- Add BodyFilterForm dialog for configuring body filters (free text, JSON path, SQL property filter)
- Support case-sensitive toggle and auto-discovered JSON paths
- Cache message bodies for filtering performance
- Apply to both queue (HandleQueueControl) and subscription (HandleSubscriptionControl) views
@SeanFeldman
Copy link
Copy Markdown
Collaborator

@claude

@claude
Copy link
Copy Markdown

claude Bot commented Apr 21, 2026

Claude finished @SeanFeldman's task in 2m 10s —— View job


Code Review

  • Gather context and read changed files
  • Review BodyFilterForm.cs (new file)
  • Review HandleQueueControl.cs changes
  • Review HandleSubscriptionControl.cs changes
  • Post comprehensive review feedback

Good feature idea — body content filtering is a real pain point for Service Bus debugging. The overall approach (inline search box + a richer dialog for JSON/property filters) is sensible. There are several issues that should be addressed before merging, ranging from a cache stale-data bug to significant code duplication.


Bugs

1. messageBodyCache is never cleared when messages are reloaded

deadletterBodyCache.Clear() is correctly called when deadletter messages are refreshed (lines 1669, 1916 of HandleQueueControl.cs), but messageBodyCache has no equivalent call when messageBindingList is reassigned (lines 1424, 1535). After a refresh, the new BrokeredMessage objects won't be found in the cache (EnsureMessageBodyCache will re-populate them), but old entries from the previous load remain, leaking memory and holding references to disposed message objects. Fix this →

2. GetJsonValueAtPath — wrong argument in recursive array search

In BodyFilterForm.cs, the array branch calls:

found = GetJsonValueAtPath(item, part);  // passes only current segment

This should pass the remaining path, not just part. For a path like orders.customer.name, when the top level is an array, it navigates into each item using only orders rather than the full remaining path. Fix this →


Significant Issues

3. Massive code duplication between HandleQueueControl and HandleSubscriptionControl

EnsureMessageBodyCache, MatchesBodyFilter, HasBodyFilter, HasAnyDeadletterBodyFilter, AddInlineSearchBox, AddDeadLetterReasonSearchBox, all the inline filter timer wiring, and the filter-dialog opening logic are copy-pasted nearly verbatim into both files (~300 lines each). Any future fix to one must also be made in the other. These belong in a shared base class or static helper utility class.

4. Custom JSON parser instead of System.Text.Json

The project already uses System.Text.Json (see HandleEventGridSubscriptionControl.cs). The PR re-implements a manual, ad-hoc JSON parser (~200 lines) that handles only a subset of JSON correctly. Known gaps in the custom parser:

  • ParseJsonObject strips outer braces with Substring(1, json.Length - 2) without verifying the last char is }
  • Unicode escapes (\uXXXX), multi-line strings, and numbers in scientific notation aren't handled
  • Errors are silently swallowed

Replace ExtractJsonPaths and GetJsonValueAtPath with System.Text.Json.JsonDocument. It's already in the SDK, handles all valid JSON, and is substantially faster.

5. Timer instances are never disposed

messagesInlineFilterTimer, deadletterInlineFilterTimer, and deadletterReasonFilterTimer are IDisposable but are never added to the form's components collection or disposed in Dispose(). This leaks the native timer handles. Fix this →

6. ToolTip in AddDeadLetterReasonSearchBox is never disposed

var tooltip = new ToolTip();
tooltip.SetToolTip(txt, "...");
// tooltip goes out of scope, never disposed

ToolTip is IDisposable. It should be tracked or added to a components container.

7. UI-blocking body reads when opening the filter dialog

EnsureMessageBodyCache is called synchronously on the UI thread every time the user clicks the filter button. For a large message set, deserializing all bodies before showing the dialog will freeze the UI. Consider showing a progress dialog or doing this lazily/on a background thread.


Minor Issues

8. BodyFilterRemovedMessage constant is declared but never used in both HandleQueueControl.cs and HandleSubscriptionControl.cs. Either use it (wire it to when the filter is cleared) or remove it.

9. Placeholder text pattern is fragile

Using txt.ForeColor == SystemColors.GrayText as the signal for "placeholder is showing" is an appearance-based check that can break under custom themes. A simple bool _isPlaceholder flag per TextBox would be more reliable.

10. BtnClear_Click sets DialogResult = DialogResult.OK

Both "Apply" and "Clear All" close the dialog with OK. The calling code can't distinguish "user cleared" from "user applied (with empty values)". While functionally equivalent here, a dedicated DialogResult extension or a separate Cleared property would make intent clearer.

11. partial class BodyFilterForm with no .Designer.cs

The class is marked partial but there's no companion .Designer.cs or .resx file, and InitializeComponent() is never called. The VS WinForms designer won't be able to open this form. Either drop partial, or provide the designer files.

12. Behavior change to PeekBatch loop unrelated to filtering

The diff introduces a batchSize variable and changes the while loop condition to while (all || totalRetrieved < count) in both control files. This is a separate behavioral change (affects how messages are retrieved when "get all" mode is active) bundled into a filtering feature PR. It should be split out or at minimum explicitly documented.

13. null! usage on string fields

private string messagesBodyFreeTextFilter = default!;  // default! for string is null

This suppresses the nullable warning but the field can be null at runtime. The IsNullOrWhiteSpace guards in the filter logic handle this safely, but initializing to string.Empty would be cleaner and avoid the ! suppression.


Summary

Severity Count
Bug 2
Significant 6
Minor 5

The core feature logic is sound, but I'd recommend addressing at minimum the cache-not-cleared bug (#1), the JSON navigation bug (#2), the timer disposal (#5), and swapping the custom JSON parser for System.Text.Json (#4) before merging. The code duplication (#3) is a maintainability concern worth tackling too.

- Fix bug: clear messageBodyCache when messages are reloaded (paolosalvatori#1)
- Fix bug: replace custom JSON parser with Newtonsoft.Json.Linq,
  fixing incorrect recursive path navigation in arrays (paolosalvatori#2, paolosalvatori#4)
- Extract shared filtering logic to MessageBodyFilterHelper to
  eliminate code duplication between controls (paolosalvatori#3)
- Dispose Timer and ToolTip instances in Dispose() (paolosalvatori#5, paolosalvatori#6)
- Remove unused BodyFilterRemovedMessage constant (paolosalvatori#8)
- Use Tag-based placeholder tracking instead of ForeColor check (paolosalvatori#9)
- Add WasCleared property to BodyFilterForm for clear vs apply (paolosalvatori#10)
- Remove partial keyword from BodyFilterForm (paolosalvatori#11)
- Revert unrelated PeekBatch behavioral change (paolosalvatori#12)
- Initialize string fields to string.Empty instead of default! (paolosalvatori#13)
- Show wait cursor during body cache population (paolosalvatori#7)
@simonhedlund-exs
Copy link
Copy Markdown
Author

@SeanFeldman
Addressed all review feedback in the latest commit. Key changes:

Replaced custom JSON parser with Newtonsoft.Json.Linq
Extracted shared code to MessageBodyFilterHelper
Fixed cache/disposal bugs, reverted unrelated PeekBatch change
Build passes, all existing tests green.

@ErikMogensen ErikMogensen changed the title feat: Add body content filtering to message and deadletter views Add body content filtering to message and deadletter views Apr 21, 2026
@ErikMogensen
Copy link
Copy Markdown
Collaborator

Wonderful, just what I needed 😄! I checked out your branch and started filtering. However, the debugger raised the following issue;

Managed Debugging Assistant 'ContextSwitchDeadlock'
Message=Managed Debugging Assistant 'ContextSwitchDeadlock' : 'The CLR has been unable to transition from COM context 0x960c2bd0 to COM context 0x960c2aa8 for 60 seconds. The thread that owns the destination context/apartment is most likely either doing a non pumping wait or processing a very long running operation without pumping Windows messages. This situation generally has a negative performance impact and may even lead to the application becoming non responsive or memory usage accumulating continually over time. To avoid this problem, all single threaded apartment (STA) threads should use pumping wait primitives (such as CoWaitForMultipleHandles) and routinely pump messages during long running operations.'

It was kind enough to offer a fix which I accepted. I'll try to create a PR to your branch with the proposed fix in a couple of hours.

@SeanFeldman
Copy link
Copy Markdown
Collaborator

@claude review again. See what could cause ContextSwitchDeadlock.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 21, 2026

Claude finished @SeanFeldman's task in 3m 12s —— View job


ContextSwitchDeadlock Root Cause Analysis

  • Gather context
  • Read MessageBodyFilterHelper.cs
  • Read BodyFilterForm.cs
  • Read HandleQueueControl.cs changes
  • Read HandleSubscriptionControl.cs changes
  • Identify ContextSwitchDeadlock causes

Cause

The ContextSwitchDeadlock MDA fires when a COM context transition (e.g. from an MTA thread to the UI's STA) cannot complete within 60 seconds because the target STA thread is blocked and not pumping Windows messages. The new filtering code introduces exactly this pattern.

The blocking call chain on the UI thread

Every time the user types a character in the inline search box (after the 400 ms timer), or clicks the filter button, this chain executes synchronously on the WinForms STA/UI thread:

Timer.Tick (UI thread)
  → ApplyInlineMessagesFilter()
    → FilterMessages()
      → EnsureMessageBodyCache()         ← loops over every uncached message
        → serviceBusHelper.GetMessageText(msg, ...)   ← called per-message
          → msg.Clone()
          → inboundMessage.GetBody<Stream>()
          → new BinaryMessageEncodingBindingElement()  ← WCF factory, created per message
          → encoderFactory.Encoder.ReadMessage(stream) ← WCF message decode

GetMessageText (ServiceBusHelper.cs:3495) constructs a BinaryMessageEncodingBindingElement, calls CreateMessageEncoderFactory(), and then encoder.ReadMessage() for every message in the binding list. This is full WCF message infrastructure — COM-aware, expensive, and recreated from scratch for every message. When there are dozens or hundreds of messages, this loop blocks the UI thread for many seconds without ever yielding to the Windows message pump.

During that time any other COM component (WCF channel infrastructure, the Service Bus SDK itself) that needs to marshal a call into the STA context will queue up and eventually time out at the 60-second threshold, emitting the ContextSwitchDeadlock warning.

The same pattern occurs in the filter button click handlers in both HandleQueueControl.cs and HandleSubscriptionControl.cs (the explicit Cursor.Current = Cursors.WaitCursor block), and in FilterDeadletters().


Why the existing code doesn't trigger this

The rest of the application loads messages synchronously on the UI thread too (no Task.Run in GetMessages()), but it does not call GetMessageText during that loading phase — the grid shows only metadata (MessageId, sequence number, etc.). Body decoding only happened when the user explicitly selected a single message. The new feature changed this by decoding every message body as part of filtering.


Fix

Recommended: Populate the cache immediately after messages load

Move the EnsureMessageBodyCache call to right after messageBindingList is assigned in GetMessages(). This consolidates body decoding into the existing "loading" phase (where a WaitCursor is already shown), so by the time the user types in the filter box, all bodies are already in the cache and FilterMessages() reads from the Dictionary — no WCF calls, no blocking, no deadlock.

In HandleQueueControl.cs (and the equivalent in HandleSubscriptionControl.cs), add immediately after each messageBindingList = new SortableBindingList<BrokeredMessage>(...):

messageBodyCache.Clear();
messageBindingList = new SortableBindingList<BrokeredMessage>(brokeredMessages) { ... };

// Pre-populate body cache while we're already on the "loading" path
MessageBodyFilterHelper.EnsureMessageBodyCache(
    messageBindingList, messageBodyCache,
    msg => serviceBusHelper.GetMessageText(msg, MainForm.SingletonMainForm.UseAscii, out _),
    writeToLog);

Then remove the EnsureMessageBodyCache calls inside FilterMessages(), FilterDeadletters(), and the filter button click handlers. Those paths will now just read from a pre-populated Dictionary — O(1) lookup, no WCF, no blocking.

Do the same for the deadletter cache after each deadletterBindingList assignment. Fix this →


Secondary issue: BinaryMessageEncodingBindingElement recreated per message

Inside GetMessageText, a new BinaryMessageEncodingBindingElement and MessageEncoderFactory are created on every call. This is WCF infrastructure that is expensive to instantiate. Even after the above fix reduces the number of calls to once-per-load, it's worth noting this can still make the load phase slow for large message sets. This is pre-existing behavior unrelated to this PR, but it compounds the problem.


Summary

Location Issue
EnsureMessageBodyCache in FilterMessages() / FilterDeadletters() Called on UI thread per filter invocation — blocks STA, prevents COM message pumping
EnsureMessageBodyCache in filter button click handlers Same issue, explicitly acknowledged with WaitCursor but still blocks STA
GetMessageText per message Creates WCF encoder factory and decodes message body — COM-aware, expensive

The fix is to pre-populate the cache at message-load time (where blocking is already expected) rather than lazily during filtering (where it blocks the message pump).

@ErikMogensen
Copy link
Copy Markdown
Collaborator

ErikMogensen commented Apr 21, 2026

Hmmm, that's a different approach that I got. In my case it put the work in an asynchronous task to offload the UI thread. If the fix Claude suggested in this thread works I think that's a better solution. @simonhedlund-exs, wanna try the the fix this link from above?

…dlock

Move EnsureMessageBodyCache calls from FilterMessages/FilterDeadletters
and filter button click handlers to immediately after messageBindingList
and deadletterBindingList are assigned during message loading.

This prevents the UI thread from being blocked by expensive WCF
GetMessageText calls during filtering, which caused a 60-second
ContextSwitchDeadlock MDA warning when COM context transitions
could not complete.
@simonhedlund-exs
Copy link
Copy Markdown
Author

@ErikMogensen
I went with the approach Claude suggested — moved EnsureMessageBodyCache to immediately after messageBindingList/deadletterBindingList are assigned during the loading phase, and removed the calls from FilterMessages(), FilterDeadletters(), and the filter button click handlers.

This way the cache is pre-populated while the WaitCursor is already showing during load, and filtering just reads from the Dictionary — no WCF calls, no blocking, no deadlock.

Pushed in commit 6a57c5d. Let me know if you still see the ContextSwitchDeadlock after this change!

@ErikMogensen
Copy link
Copy Markdown
Collaborator

ErikMogensen commented Apr 22, 2026

@simonhedlund-exs, I ran the PR through Copilot and it found the following issues:

High

  1. Body cache is populated unconditionally at load time, even without an active filter. Every peek/receive now calls GetMessageText on every message (WCF XML decode, potential gzip decompress, BrokeredMessage.Clone() even if the user never touches the body filter. For 500 messages this adds significant overhead to every refresh.

  2. Log flooding on every keypress is still present. writeToLog(string.Format(BodyFilterAppliedMessage, ...) fires inside FilterMessages/FilterDeadletters on every debounce tick. Typing a 6-character search term produces 6 log entries.

Medium

  1. BodyFilterForm uses hard-coded absolute pixel coordinates throughout. All controls placed with new Point(x, y) — breaks on high-DPI displays. FormBorderStyle.FixedDialog prevents resizing, but buttons are anchored Bottom | Right while their Location is fixed, so they'll drift incorrectly if the DPI scaling changes the client area.

  2. Tag ordering bug in placeholder restore. In AddInlineSearchBox (and AddDeadLetterReasonSearchBox), the LostFocus handler sets txt.Text = "Search body..." before txt.Tag = true. This fires TextChanged with Tag still false, triggering the debounce timer and an unnecessary filter cycle.

  3. Bare catch blocks. ExtractJsonPaths and GetJsonValueAtPath use catch { } and catch { return null; } which silently swallow OutOfMemoryException and ThreadAbortException. Should be catch (JsonException).

Low

  1. WasCleared is dead code. Set in BtnClear_Click but never read by either calling control.

  2. PopulateDeadLetterReasonCombo is misleadingly named. It only subscribes a TextChanged event on a TextBox — no combo, no population.

  3. Redundant Close() calls. Setting DialogResult on a modal form already closes it; the explicit Close() in BtnOk_Click and BtnClear_Click is a no-op.

  4. ~110 lines of code duplicated verbatim across HandleQueueControl and HandleSubscriptionControl (filter fields, timer wiring, Apply*Filter methods, HasAnyDeadletterBodyFilter, additions).

My comments to the above points
You can ignore number 2. I believe the reason for number 3 is that you ran Visual Studio as a DPI-aware process.

It is hard getting a PR approved these days...

@ErikMogensen
Copy link
Copy Markdown
Collaborator

@simonhedlund-exs, I used your updated PR today and it solved my problem 😃. Thanks 🙏🏻 !

@ErikMogensen
Copy link
Copy Markdown
Collaborator

I suggest to solve the first one by reading the message bodies during the first search and then storing them in a cache.

@ErikMogensen
Copy link
Copy Markdown
Collaborator

I realize now that the maximum queue size is 80 GB which would break most machines if it was stored in the cache. That complicates the caching 😢

Remove unconditional body cache population at load time.
Bodies are now decoded on demand, on a background thread, the
first time a body filter is applied. The UI thread is never
blocked; FilterMessages/FilterDeadletters re-runs automatically
when the cache is ready via ContinueWith on the UI context.

A version counter (_messageCacheVersion) prevents stale cache
completions from a superseded message load from affecting the
current view.

The filter dialog (pictFind) uses synchronous population since
the user explicitly clicked — a brief wait is expected there.

Also fix:
- Tag set before Text in LostFocus placeholder handlers to
  prevent spurious debounce timer trigger on blur
- catch {} -> catch (Exception) in BodyFilterForm JSON helpers
- Remove redundant Close() calls after setting DialogResult
- Remove dead WasCleared property from BodyFilterForm
@simonhedlund-exs
Copy link
Copy Markdown
Author

@ErikMogensen Went through all the points and pushed a new commit.

The biggest change is the cache strategy — you were right that pre-populating on every load was wasteful. Bodies now get decoded on a background thread only when a filter is actually applied, and the filter just re-runs automatically when it's done. Added a version counter so a slow background task from an old load can't corrupt the current view.

Also fixed the LostFocus placeholder issue (was setting Text before Tag, which would fire the debounce timer on blur), swapped out the bare catch {} blocks, removed the redundant Close() calls in the dialog, and cleaned up the dead WasCleared property.

Let me know if anything looks off!

@ErikMogensen
Copy link
Copy Markdown
Collaborator

My Copilot says:

Review: feature/body-search-filter (commit 99a8b5a)

This covers the latest commit — perf: Cache message bodies lazily on first filter use — and includes issues carried forward from earlier commits that remain unresolved.


1. Dictionary race condition — background thread writes while UI thread reads 🔴

capturedCache in the Task.Run lambda and messageBodyCache on the UI thread are the same Dictionary<BrokeredMessage, string> instance. Dictionary<> is not thread-safe. If the user opens the filter dialog while a background populate is still running, the dialog-open handler immediately calls the synchronous EnsureMessageBodyCache on the UI thread — writing to the same dictionary simultaneously from two threads. This can silently corrupt the dictionary's internal state.

// Task.Run background thread — writes capturedCache (same ref as messageBodyCache):
capturedCache[msg] = serviceBusHelper.GetMessageText(msg, useAscii, out _);

// UI thread in filter button click — also writes messageBodyCache concurrently:
if (!_messageCacheReady)
{
    MessageBodyFilterHelper.EnsureMessageBodyCache(messageBindingList, messageBodyCache, ...);

Suggestion: Use ConcurrentDictionary<BrokeredMessage, string>, or set _messageCachePopulating = true before starting the task and skip the synchronous fallback while population is in progress.


2. Filter dialog blocks the UI thread with no feedback 🔴

The filter button click handler falls through to a synchronous EnsureMessageBodyCache call when _messageCacheReady is false. This blocks the UI thread for as long as it takes to decode all message bodies, with no cursor change or progress indicator. The previous commit had Cursor.Current = Cursors.WaitCursor which was removed here.

if (!_messageCacheReady)
{
    // Blocks UI thread — no cursor, no progress bar, no cancellation
    MessageBodyFilterHelper.EnsureMessageBodyCache(messageBindingList, messageBodyCache,
        msg => serviceBusHelper.GetMessageText(msg, MainForm.SingletonMainForm.UseAscii, out _), writeToLog);
    _messageCachePopulating = false;
    _messageCacheReady = true;
}

Suggestion: If the async cache is still running, disable the filter button and re-enable it in the onReady callback, or show a brief status message and return early.


3. catch (Exception) in background task silently swallows all errors 🟡

The catch block inside Task.Run catches Exception but does nothing except store an empty string. This means OutOfMemoryException, corruption, and unexpected errors are silently hidden with no log output — unlike the synchronous EnsureMessageBodyCache in MessageBodyFilterHelper which at least calls writeToLog.

catch (Exception)
{
    capturedCache[msg] = string.Empty;
}

Suggestion: Catch only expected types (InvalidOperationException, IOException), or at minimum log the failure:

catch (Exception ex)
{
    writeToLog($"[BodyFilter] Failed to decode body for {msg.MessageId}: {ex.Message}");
    capturedCache[msg] = string.Empty;
}

4. Log flooding on every debounce tick 🟡

writeToLog(string.Format(BodyFilterAppliedMessage, filteredList.Count)) fires inside FilterMessages and FilterDeadletters on every debounce timer tick. Typing a six-character search term produces six log entries. This pollutes the log and can cause noticeable UI lag on large message sets.

if (hasBodyFilter)
{
    filteredList = filteredList.Where(...).ToList();
    writeToLog(string.Format(BodyFilterAppliedMessage, filteredList.Count)); // fires on every keypress
}

Suggestion: Only log when the filter is applied via the explicit "Apply" button, not during inline live search.


5. BodyFilterForm uses hard-coded absolute pixel coordinates throughout 🟡

All controls are positioned with new Point(x, y) and sized with new Size(w, h). This is inconsistent with the rest of the codebase and breaks layout on high-DPI displays (125%, 150%, 200% scaling).

grpPropertyFilter = new GroupBox { Location = new Point(12, 12), Size = new Size(480, 60), ... };
txtPropertyFilter  = new TextBox  { Location = new Point(90, 22), Size = new Size(375, 20), ... };
btnOk              = new Button   { Location = new Point(310, 345), Size = new Size(85, 30), ... };

Suggestion: Use a .resx / Designer file with AutoScaleMode = AutoScaleMode.Dpi, or use layout panels (FlowLayoutPanel, TableLayoutPanel) so the form scales correctly.


6. catch (Exception) in ExtractJsonPaths and GetJsonValueAtPath 🟡

Both static helpers catch Exception broadly instead of catching only JsonException (or Newtonsoft.Json.JsonReaderException). This masks bugs and can hide fatal CLR exceptions.

catch (Exception)
{
    // Not valid JSON, skip
}

Suggestion: Use catch (Newtonsoft.Json.JsonException) to limit the scope to expected parse failures.


7. PopulateDeadLetterReasonCombo is misleadingly named 🟢

The method does not populate anything. It only subscribes a TextChanged event on a TextBox. The name implies it fills a combo box.

private void PopulateDeadLetterReasonCombo()
{
    if (txtDeadLetterReasonFilter == null) return;
    txtDeadLetterReasonFilter.TextChanged -= OnDeadLetterReasonTextChanged;
    txtDeadLetterReasonFilter.TextChanged += OnDeadLetterReasonTextChanged;
}

Suggestion: Rename to SubscribeDeadLetterReasonFilter or WireDeadLetterReasonFilter.


8. ~100 lines of code duplicated verbatim across two controls 🟢

HandleQueueControl and HandleSubscriptionControl contain identical copies of:

  • EnsureMessageBodyCacheAsync / EnsureDeadletterBodyCacheAsync
  • ApplyInlineMessagesFilter / ApplyInlineDeadletterFilter / ApplyDeadLetterReasonFilter
  • OnMessagesInlineFilterChanged / OnDeadletterInlineFilterChanged
  • All filter state fields (_messageCacheReady, messagesBodyFreeTextFilter, etc.)

Any future bug fix or behaviour change must be applied in both places.

Suggestion: Extract a MessageBodyFilterState class (holding the cache, version counter, and filter values) and a shared MessageBodyFilterController that owns the async cache methods, then compose it into both controls.

My comments to the above

@simonhedlund-exs, you can ignore number 4 and 8, but please fix the others.

There is also the issue of searching through 80 GB of messages which would freeze most computers. I suggest you check the size of the cache and stop adding to it if it is larger than 2 GB.

…tCursor, 2GB cap, DPI scaling, JsonException, WireDeadLetterReasonFilter rename
@simonhedlund-exs
Copy link
Copy Markdown
Author

Hi @ErikMogensen , thanks for the feedback! Fixed everything in 949c4d3:

  • ConcurrentDictionary for both caches (queue + subscription)
  • writeToLog moved out of Task.Run — collected in a local list and flushed in ContinueWith on the UI thread
  • TryAdd instead of ContainsKey + indexer
  • 2 GB cap with a log message when hit
  • WaitCursor on the sync populate calls
  • AutoScaleMode.Dpi on BodyFilterForm
  • catch (JsonException) instead of catch (Exception)
  • Renamed PopulateDeadLetterReasonComboWireDeadLetterReasonFilter

Let me know if anything else needs changing!

@ErikMogensen
Copy link
Copy Markdown
Collaborator

Thank you @simonhedlund-exs, I ran it through my local Copilot and got the following:

Code Review: feature/body-search-filter

Issues

1. Significant code duplication — EnsureMessageBodyCacheAsync / EnsureDeadletterBodyCacheAsync

Both methods in HandleQueueControl (and again in HandleSubscriptionControl) are ~80 lines of near-identical code. The only differences are which binding list, cache, flags, and version counter are used. This results in roughly 320 lines of duplication across the two controls.

Suggestion: Extract into a reusable class (e.g., MessageBodyCache) that encapsulates the state and async population logic, then use instances for messages and dead-letters:

// replaces 4 copies of EnsureMessageBodyCacheAsync
private readonly MessageBodyCache _messageCache = new MessageBodyCache();
private readonly MessageBodyCache _deadletterCache = new MessageBodyCache();

2. Font objects leak GDI handles — MessageBodyFilterHelper

AddInlineSearchBox and AddDeadLetterReasonSearchBox both create new Font("Microsoft Sans Serif", 8.25F). WinForms TextBox does not take ownership of assigned fonts; these objects are never disposed.

// Leaks a Font GDI handle:
Font = new Font("Microsoft Sans Serif", 8.25F),

Suggestion: Use parent.Font or the system default font instead, or dispose the font when the parent control is disposed.


3. BodyFilterForm not using WinForms designer — inconsistent with codebase

The new form builds its UI entirely in InitializeForm(). All other forms in this project use .Designer.cs files. This makes it impossible to visually edit in the designer and harder to maintain.

Suggestion: Move to the standard WinForms designer pattern (a BodyFilterForm.Designer.cs file with InitializeComponent()).


4. JSON path with dot-notation silently fails for keys containing dots

GetJsonValueAtPath splits the path on '.', so any JSON key that itself contains a dot (e.g. "data.v1") will silently produce no match. Users have no indication that their path failed vs. simply not matching.

Suggestion: Either document this limitation clearly in the UI tooltip/hint text, or use SelectToken from Json.NET which supports proper JSONPath syntax natively:

// Handles nested paths, arrays, and keys with dots correctly:
var result = jobj.SelectToken(path);

5. EnsureMessageBodyCache (sync) blocks the UI thread

The synchronous cache pre-population in MessageBodyFilterHelper.EnsureMessageBodyCache is called with Cursor.Current = Cursors.WaitCursor but has no timeout or max-size guard — unlike the async version which caps at 2 GB and uses a background thread. For large message sets, opening the filter dialog can freeze the UI.

Suggestion: Use consistent behavior — either always use the async path (populate in the background before showing the dialog), or add the same 2 GB cap and early-exit to the sync path.


6. _messageCachePopulating flag is not guarded against re-entrant calls

If FilterMessages() is called while _messageCachePopulating = true, it returns without scheduling a new callback. If the filter state changes again (e.g. user clears the inline filter then retypes), the re-entrant call is silently dropped. The version counter protects against stale completions, but the dropped re-entrant call is unrecovered.

Suggestion: When returning false from EnsureMessageBodyCacheAsync because a population is already in-flight, store the latest onReady callback and invoke it (instead of the original one) when population completes.


7. Minor: BtnClear_Click sets DialogResult directly

private void BtnClear_Click(object sender, EventArgs e)
{
    ...
    DialogResult = DialogResult.OK;
}

Setting this.DialogResult closes a modal form (intended behavior), but it bypasses AcceptButton/CancelButton semantics and is non-obvious.

Suggestion: Assign this.DialogResult and call this.Close() explicitly, or wire the button's DialogResult property in the constructor and only set the output properties in BtnOk_Click.


What's Done Well

  • Async body cache with version counter correctly prevents stale completions after a message list refresh.
  • Debounced timers (400ms) for inline search prevent excessive re-filtering on each keystroke.
  • WireDeadLetterReasonFilter uses unsubscribe-then-subscribe pattern to prevent double-wiring.
  • 2 GB cache cap in the async path prevents OOM on large namespaces.
  • JsonException is caught specifically rather than swallowing all exceptions.
  • ConcurrentDictionary is used correctly for the cache shared between UI and background threads.

My comments to the review above

You can ignore the first one about code duplication, I can create a separate PR that. Please fix the other ones though unless you object...

simonhedlund-exs and others added 3 commits May 4, 2026 09:54
- Remove Font leak in MessageBodyFilterHelper (GDI handle leak)
- Replace manual JSON path parsing with SelectToken (handles dots in keys)
- Add 2GB cache cap to sync EnsureMessageBodyCache (consistency with async)
- Implement re-entrancy callback storage for better UX during cache population

Resolves review comments from Erik Mogensen.

Co-authored-by: Copilot <copilot@github.com>
Add explicit Close() call after setting DialogResult in BtnClear_Click
for clarity. While setting DialogResult closes a modal form implicitly,
the explicit Close() makes the intent clearer.

Addresses review feedback paolosalvatori#7 from PR paolosalvatori#871
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants