fix: bypass render throttle for streaming content on mobile

[PureWeen]

Problem

Mobile users see no streaming content — messages only appear after manually hitting the sync/refresh button. The WsBridge connection is active and content_delta events are being received and stored in session history, but the UI never re-renders.

Root Cause

The 500ms render throttle in RefreshState() blocks content rendering on mobile:

1. HandleContent() receives content_delta → calls ScheduleRender() (50ms timer)
2. Timer fires → SafeRefreshAsync() → RefreshState()
3. _refreshThrottle.ShouldRefresh(false, false) → returns false (< 500ms since last refresh)

On desktop, this is masked because SDK events fire frequent OnStateChanged calls that consume the throttle window through different code paths. On mobile (remote mode), content_delta is the only event source — and every render attempt hits the 500ms wall.

Fix

• Added isStreaming parameter to RenderThrottle.ShouldRefresh() that bypasses the 500ms throttle
• Added _contentDirty flag in Dashboard.razor, set by HandleContent, consumed by RefreshState to signal active streaming
• 2 new tests for streaming throttle bypass

Changes

File	Change
RenderThrottle.cs	Added isStreaming parameter that bypasses throttle
Dashboard.razor	Added _contentDirty flag, wired into HandleContent → RefreshState
RenderThrottleTests.cs	2 new tests

All 2993 tests pass.

[PureWeen]

Review: PR #447 — fix: bypass render throttle for streaming content on mobile

Summary

Clean, minimal fix. The root cause analysis is correct: HandleContent → ScheduleRender → SafeRefreshAsync → RefreshState → ShouldRefresh(false, false) returns false every time when content_delta is the only event source (remote mode). The fix is well-scoped to the actual problem.

---

✅ What's correct

• Root cause is accurately diagnosed. On desktop, OnStateChanged fires from many SDK events, so renders happen from other paths. On mobile (remote mode), OnContentReceived → HandleContent is the only event source during streaming, and the 500ms throttle blocks every render attempt.
• _contentDirty as a volatile bool is the right mechanism. It bridges the gap between the background-thread HandleContent call and the UI-thread RefreshState read. The read-then-clear pattern (var isStreaming = _contentDirty; if (isStreaming) _contentDirty = false;) is safe — worst case, an extra streaming render fires (benign).
• isStreaming is defaulted to false in ShouldRefresh — correct, no breaking change to existing callers.
• Streaming bypass updates _lastRefresh — correct, consistent with the hasCompletedSessions bypass. This means a non-streaming render immediately after streaming won't fire unnecessarily.
• Tests are accurate — both new tests correctly verify bypass behavior and timestamp update semantics.

---

⚠️ Minor: streaming still throttled by the 50ms ScheduleRender timer

HandleContent calls ScheduleRender(), which sets _renderDirty = true and resets a 50ms timer. Multiple rapid content_delta events within the 50ms window collapse into a single render (the timer is reset on each call via Change(50, Infinite)). This is intentional and good — it batches burst deltas.

However, the current fix will _contentDirty = false in RefreshState after the first successful render, then if another content_delta fires before the next 50ms timer fires, _contentDirty is set again correctly. This is fine — the flag is re-set by each HandleContent call.

No action needed, just confirming the interaction is correct.

---

⚠️ Minor: _contentDirty is cleared even when RefreshState early-exits

There are two early exits before the throttle check in RefreshState:

if (_refreshing) return;   // line 1261

The _contentDirty read-and-clear happens after these guards:

var isStreaming = _contentDirty;
if (isStreaming) _contentDirty = false;

So if RefreshState is re-entered (_refreshing guard) or fires before init, the flag is NOT prematurely cleared — it's preserved for the next call. This is correct behavior.

---

✅ No performance regression concern

Streaming renders were already being triggered via ScheduleRender → 50ms timer → SafeRefreshAsync. The fix doesn't add any new timer or render schedule — it only removes the throttle gate that was blocking renders already in flight. The _lastRefresh update in the streaming path means normal state-change renders (OnStateChanged) right after streaming are still throttled, preventing any amplification.

---

Verdict

✅ Approve. The fix is correct, minimal, safe, and well-tested. The root cause is accurately identified and the change directly addresses it without side effects.

[PureWeen]

🔍 Squad Review — PR #447

PR: fix: bypass render throttle for streaming content on mobile
Size: +44/-5, 3 files
Commit: 28632281
Tests: ✅ 10/10 RenderThrottle tests pass (confirmed locally)

---

Root Cause & Fix

Correct diagnosis. content_delta events on mobile (remote mode) are dispatched via SyncContext.Post → HandleContent → ScheduleRender() (50ms timer). When the timer fires, RefreshState() checks ShouldRefresh(false, false) — returns false if fewer than 500ms have elapsed since the last allowed render. On desktop, SDK events fire OnStateChanged frequently enough to open that window naturally; on mobile the content_delta path never resets _lastRefresh.

The fix is targeted and correct:

1. _contentDirty flag set in HandleContent, cleared atomically in RefreshState
2. isStreaming bypass added to RenderThrottle.ShouldRefresh() with _lastRefresh reset
3. Two new tests verify bypass and throttle-reset behavior

---

Thread Safety ✅

HandleContent and RefreshState both run on the UI thread (confirmed: OnContentReceived is dispatched via _syncContext.Post in Events.cs:313). Both reads and writes to _contentDirty are therefore single-threaded in practice. The volatile keyword is correct — it provides visibility guarantee for any future non-UI-thread reader — and consistent with the existing _renderDirty volatile bool on the adjacent line.

---

Atomicity of Read-Clear ✅

var isStreaming = _contentDirty;
if (isStreaming) _contentDirty = false;

Because both HandleContent (writer) and RefreshState (reader/clearer) run on the UI thread, there's no race between set and clear. If HandleContent fires again between the read and the clear in RefreshState, the worst outcome is that the flag is cleared with a pending delta — but ScheduleRender() already re-scheduled the 50ms timer for that delta, so the next timer firing will set _contentDirty = true again and proceed. No content is lost.

---

Throttle Reset Behavior ✅

isStreaming updates _lastRefresh = now, so rapid consecutive deltas each reset the throttle clock. This means non-streaming OnStateChanged events are throttled to 500ms after the last delta — correct behavior, preventing redundant renders after streaming ends.

---

No Desktop Regression Risk ✅

_contentDirty is only set inside HandleContent. On desktop, the SDK fires AssistantMessageDeltaEvent which also triggers OnStateChanged, which already drives renders at full speed via a different path. The isStreaming bypass is additive — it doesn't change the desktop code path.

---

Minor Observations (non-blocking)

🟢 Nit: HandleContent only sets _contentDirty for the expanded session path

streamingBySession[sessionName] += content;
_contentDirty = true;  // set unconditionally
if (sessionName == expandedSession || expandedSession == null)
    ScheduleRender();   // but ScheduleRender is conditional

_contentDirty = true fires for all sessions including non-expanded ones. But ScheduleRender only fires when the session is active/expanded. So _contentDirty may be set but the render timer isn't scheduled — RefreshState won't be called to consume the flag. In practice this is fine (non-expanded sessions aren't visible), but it means the flag can linger until the next unrelated render. Not a bug.

🟢 Nit: Streaming_UpdatesLastRefreshTime test name could be more descriptive

The test verifies that a streaming bypass throttles subsequent normal renders. Name like Streaming_BypassResetsThrottle_BlocksImmediateNormalRefresh would be more self-documenting. Cosmetic only.

---

✅ Verdict: Approve

Correct fix for a real mobile streaming regression. Thread safety is sound (both accesses are UI-thread). Two new tests cover the key behaviors. No desktop regression risk. Minor observations are cosmetic and non-blocking.

🚢 Good to merge.

[PureWeen]

🤖 PR #447 Review

Title: fix: bypass render throttle for streaming content on mobile
Files changed: 3 files (+44, -5 lines)
Test results: ✅ RenderThrottleTests: 12/12 pass (2 new tests added)
Overall tests: 2992/2993 pass (1 unrelated intermittent failure in DiagnosticsLogTests)

---

✅ Problem Statement — Clear and Critical

User-facing symptom: Mobile users see no streaming content — messages only appear after manually hitting sync/refresh.

Root cause analysis (excellent):

1. content_delta events are being received and stored in session history correctly
2. The 500ms render throttle in RefreshState() blocks the UI from rendering
3. On desktop, frequent SDK OnStateChanged events mask this issue
4. On mobile (remote mode), content_delta is the only event source → every render hits the 500ms wall

Why this is critical: Streaming is a core UX feature. Without it, mobile feels broken.

---

✅ Solution — Clean and Minimal

The fix adds an isStreaming bypass to the render throttle:

1. RenderThrottle.cs (3 lines changed, 11 added)

Added isStreaming parameter to ShouldRefresh():

public bool ShouldRefresh(bool isSessionSwitch, bool hasCompletedSessions, bool isStreaming = false)
{
    // ... existing session switch check ...
    
    // Always allow during active streaming — content deltas must render promptly
    if (isStreaming)
    {
        _lastRefresh = now;
        return true;
    }
    
    // ... rest of throttle logic ...
}

Why this is correct:

• ✅ Bypasses throttle when streaming is active
• ✅ Updates _lastRefresh to maintain throttle for subsequent non-streaming calls
• ✅ isStreaming is optional (defaults to false) — backward compatible

---

2. Dashboard.razor (2 lines changed, 7 added)

Added _contentDirty flag:

private volatile bool _contentDirty; // Set when content_delta arrives, cleared after render

Set in HandleContent() (line 1301):

streamingBySession[sessionName] += content;
_contentDirty = true;  // NEW
if (sessionName == expandedSession || expandedSession == null)
    ScheduleRender();

Consumed in RefreshState() (lines 1189-1191):

var isStreaming = _contentDirty;
if (isStreaming) _contentDirty = false;  // Clear after consuming
if (!_refreshThrottle.ShouldRefresh(sessionSwitched, completedSessions.Count > 0, isStreaming))
    return;

Why this is correct:

• ✅ volatile ensures visibility across threads
• ✅ Set-once, consume-once pattern prevents stale flags
• ✅ Cleared immediately after reading (line 1190) to avoid false positives on next render
• ✅ Wired through the existing ScheduleRender() → RefreshState() path

---

3. RenderThrottleTests.cs (+27 lines, 2 new tests)

Test 1: Streaming_BypassesThrottle

var throttle = new RenderThrottle(500);
Assert.True(throttle.ShouldRefresh(...)); // First call passes
Assert.False(throttle.ShouldRefresh(...)); // Immediate retry throttled
Assert.True(throttle.ShouldRefresh(..., isStreaming: true)); // Streaming bypasses

Test 2: Streaming_UpdatesLastRefreshTime

throttle.SetLastRefresh(DateTime.UtcNow.AddSeconds(-10));
Assert.True(throttle.ShouldRefresh(..., isStreaming: true)); // Streaming bypasses
Assert.False(throttle.ShouldRefresh(...)); // Normal retry still throttled

Why these tests are good:

• ✅ Test 1 verifies the bypass itself
• ✅ Test 2 verifies that streaming updates the throttle timestamp (prevents infinite bypass)
• ✅ Both tests are concise and focused

---

🟢 Code Quality Observations

1. The _contentDirty pattern is thread-safe but has a subtle race

Scenario:

1. Thread A: HandleContent() sets _contentDirty = true, calls ScheduleRender()
2. Thread B: Another HandleContent() sets _contentDirty = true, calls ScheduleRender()
3. Timer fires → RefreshState() reads _contentDirty (true), clears it
4. Render happens with isStreaming = true

Is this a problem? No, because:

• The flag is consumed on every render cycle
• Multiple content deltas within the throttle window all correctly trigger bypass
• Worst case: one extra render with isStreaming = true when no new content arrived → harmless

---

2. isStreaming parameter placement is consistent

The parameter is added after hasCompletedSessions, maintaining backward compatibility (default false). All callers that don't pass it get the old behavior.

---

3. Comment clarity is excellent

Dashboard.razor line 1021:

private volatile bool _contentDirty; // Set when content_delta arrives, cleared after render

Dashboard.razor lines 1186-1187:

// Throttle non-switch state changes to max 2/sec, but always allow completed sessions
// and active streaming content through

Both comments clearly explain the purpose and lifecycle of the flag.

---

🎯 Impact Analysis

Before this PR:

• Mobile users see no streaming content (broken UX)
• Content only appears after manual sync button click
• Every content_delta event is correctly received but UI never updates

After this PR:

• Streaming content renders immediately on mobile
• Throttle still protects against excessive renders (non-streaming events)
• Desktop behavior unchanged (SDK events were already bypassing via other paths)

---

📊 Test Coverage

Category	Before	After	Delta
RenderThrottleTests	10	12	+2
Total tests	2991	2993	+2
Passing	-	2992	-

Note on the 1 failing test:

• DiagnosticsLogTests.Debug_AllFilteredPrefixes_WriteToDiagnosticsLog fails for "[IDLE] session went idle"
• This is unrelated to this PR (no changes to diagnostic logging)
• Likely an intermittent test isolation issue (test suite has 2993 tests)
• Should be investigated separately

---

🎯 Final Verdict

✅ APPROVE — Excellent fix for a critical mobile UX issue.

Strengths:

1. ✅ Clear problem statement with root cause analysis
2. ✅ Minimal, surgical fix (3 files, 44 net lines)
3. ✅ Proper thread safety with volatile flag
4. ✅ Good test coverage (2 new focused tests)
5. ✅ Backward compatible (optional parameter with default)
6. ✅ Excellent code comments

Minor notes:

• The _contentDirty flag has a minor race condition (multiple content deltas), but it's benign (only causes one extra render)
• 1 unrelated test failure (DiagnosticsLogTests) should be fixed separately

Recommendation: Ready to merge. This fixes a show-stopping mobile UX bug with a clean, well-tested solution.

---

📈 Context

This PR is part of the ongoing mobile streaming improvements:

• PR feat: add sync button for mobile + diagnostic logging #438: Added sync button + diagnostic logging
• PR fix: force sync bypasses streaming guard for active session #444: Fixed force sync to bypass streaming guard
• PR fix: bypass render throttle for streaming content on mobile #447: Fixed render throttle blocking streaming content ← this PR

The incremental approach shows good engineering discipline — each PR addresses a specific, well-scoped issue discovered through real mobile usage.

[PureWeen]

🔍 Squad Review — PR #447 Round 1

Models: claude-opus-4.6 ×2, claude-sonnet-4.6 ×2, gpt-5.3-codex
CI: No checks reported (1 commit, fresh PR)

---

🟡 Moderate (consensus: 3/5 models)

M1 — _contentDirty set for ALL sessions, not just the visible one

// In HandleContent():
streamingBySession[sessionName] += content;
_contentDirty = true;                                    // ← unconditional
if (sessionName == expandedSession || expandedSession == null)
    ScheduleRender();                                     // ← conditional on visibility

_contentDirty is set even when content arrives for background/non-visible sessions. This causes the next RefreshState (triggered by any OnStateChanged event) to bypass the throttle for a render that has no new visible content. In multi-agent scenarios with many background workers streaming, this effectively disables the throttle globally.

Fix: Move _contentDirty = true inside the if block alongside ScheduleRender():

if (sessionName == expandedSession || expandedSession == null)
{
    _contentDirty = true;
    ScheduleRender();
}

M2 — _contentDirty cleared before ShouldRefresh — fragile ordering (consensus: 3/5 models)

var isStreaming = _contentDirty;
if (isStreaming) _contentDirty = false;  // cleared here
if (!_refreshThrottle.ShouldRefresh(sessionSwitched, completedSessions.Count > 0, isStreaming))
    return;  // if this returns, flag is lost

Currently safe because isStreaming=true guarantees ShouldRefresh returns true. But this is a hidden invariant — if ShouldRefresh ever gains a higher-priority guard that returns false before checking isStreaming, the dirty flag is consumed without rendering.

Fix: Clear the flag after confirming the render will proceed, or add a comment documenting the invariant.

---

🟢 Minor

#	Issue	Models
m1	No test for the _contentDirty integration path (background session sets flag → non-visible render bypass)	2/5
m2	isStreaming parameter couples Dashboard domain logic into RenderThrottle utility — consider keeping throttle pure	1/5

---

Verified Non-Issues

• TOCTOU on volatile bool read+clear — Both HandleContent and RefreshState run on the UI thread (marshaled via SynchronizationContext). The read+clear is effectively serialized. (3/5 models confirm)
• _lastRefresh update during streaming starving non-content UI — Session completion events bypass via hasCompletedSessions. Tool phase transitions and session list changes go through OnStateChanged which has its own ScheduleRender() path. The 50ms timer naturally caps render rate regardless. (2/5 models confirm correct)
• _lastRefresh DateTime thread safety — Only accessed from UI thread. Safe. (2/5 models confirm)

---

✅ Approve

The fix is sound and correctly addresses the mobile streaming rendering problem. M1 (flag set for non-visible sessions) is a real inefficiency in multi-agent scenarios but not a correctness bug — it causes unnecessary renders, not missing renders. M2 is a defensive coding suggestion. Neither blocks merge.

Two small improvements worth including:

1. Move _contentDirty = true inside the session-visibility guard
2. Add a comment documenting the _contentDirty → isStreaming=true → ShouldRefresh always-returns-true invariant

[PureWeen]

🔍 Squad Review -- PR #447 R2

PR: fix: bypass render throttle for streaming content on mobile
Commits: 2 (new: 5ccddc8 -- Fix stale IsProcessing on mobile after multi-agent completion)
Tests: ✅ 2994/2997 (3 pre-existing intermittent failures)

R1 Findings Status

• M1 (_contentDirty set for all sessions): STILL PRESENT (non-blocking)
• M2 (_contentDirty cleared before render confirmed): STILL PRESENT (non-blocking)

New Commit Analysis (5ccddc8)

Root cause fixed: Debounced sessions_list (500ms) could overwrite TurnEnd's authoritative IsProcessing=false with a stale snapshot, causing mobile sessions to show "busy/sending" forever after multi-agent worker completion.

Solution: _recentTurnEndSessions guard dictionary (5s expiry) prevents stale sessions_list from re-setting IsProcessing=true after TurnEnd clears it.

Correctness verified:

• ✅ Guard set AFTER clearing IsProcessing (correct order)
• ✅ All 4 processing status fields cleared together (INV-1 compliant)
• ✅ Guard cleared on TurnStart (new turns can set IsProcessing)
• ✅ SessionComplete fallback also clears IsProcessing (defense-in-depth)
• ✅ Thread-safe: ConcurrentDictionary + all mutations via InvokeOnUI
• ✅ 4 new tests covering: stale sync blocked, initial sync allowed, TurnStart clears guard, server can always clear to false

Verdict

✅ Approve -- Both fixes (R1: streaming render bypass, R2: stale IsProcessing guard) are correct, well-tested, and independent. R1 suggestions (M1/M2) are non-blocking efficiency improvements for a follow-up.