Skip to content

fix(ui): reduce UI flicker — throttle + startTransition + batch STREAM_TEXT + debounce refresh + resize settle #5396

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
aspnmy wants to merge 9 commits into from
Closed

fix(ui): reduce UI flicker — throttle + startTransition + batch STREAM_TEXT + debounce refresh + resize settle #5396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
82935c1
fix(ui): B1+A2+C1 — reduce UI flicker — throttle 60→100ms + startTran…
aspnmy2024 Jun 19, 2026
51bf0d4
test: update throttle timing in useGeminiStream tests (60→100ms) (#4561)
aspnmy2024 Jul 2, 2026
80b01cc
fix(ui): debounce refreshStatic + increase resize settle to reduce fl…
aspnmy2024 Jul 2, 2026
fdf7839
test: add STREAM_TEXT batch emit contract test (#2928)
aspnmy2024 Jul 2, 2026
cdc9399
test: add STREAM_TEXT batch emit contract test (#2928)
aspnmy2024 Jul 2, 2026
1a6916e
fix(core): resolve merge conflict in agent-core.test.ts — keep AgentE…
qwen-code-dev-bot Jul 7, 2026
0bdc5d9
fix(cli): resolve merge conflict in AppContainer.tsx — combine deboun…
qwen-code-dev-bot Jul 10, 2026
7a09466
Merge branch 'main' into fix/aspnmy-agent-cli-ui-flicker
wenshao Jul 10, 2026
6e6a57e
fix: merge main — keep refreshStatic debounce and STREAM_TEXT batchin…
qwen-code-dev-bot Aug 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions packages/cli/src/ui/AppContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1303,11 +1303,18 @@ export const AppContainer = (props: AppContainerProps) => {
),
);
const showScrollbar = settings.merged.ui?.showScrollbar ?? true;
const refreshStaticRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const refreshStatic = useCallback(() => {
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
if (refreshStaticRef.current) {
clearTimeout(refreshStaticRef.current);
}
remountStaticHistory();
refreshStaticRef.current = setTimeout(() => {
refreshStaticRef.current = null;
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
}
remountStaticHistory();
}, 0);
Comment on lines +1311 to +1317

@doudouOUC doudouOUC Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Critical] setTimeout(0) defers remountStaticHistory() (which calls setHistoryRemountKey) to a macrotask, breaking the same-commit batching invariant documented at lines 1155–1175 of this file. The onModelChange handler (line 1183) calls refreshStatic() then setCurrentModel(model) synchronously — previously both state updates landed in the same React commit. Now setCurrentModel commits first with the OLD historyRemountKey, producing a full-history Static render that bypasses progressive replay — the exact #3899 freeze regression that PR #4119 fixed.

Additionally, refreshStaticRef has no useEffect cleanup on unmount — a pending timer can fire setHistoryRemountKey on an unmounted component.

— Failure scenario: user changes model (Ctrl+M or /model) → refreshStatic() schedules macrotask → setCurrentModel commits immediately → <Static> renders with new model but old remount key → full-history flash / freeze.

Suggested change
refreshStaticRef.current = setTimeout(() => {
refreshStaticRef.current = null;
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
}
remountStaticHistory();
}, 0);
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
}
remountStaticHistory();

Keep refreshStatic synchronous. If debounce is needed for the resize path, extract a separate refreshStaticDebounced wrapper used only by those callers.

— qwen3.7-max via Qwen Code /review

}, [useTerminalBuffer, remountStaticHistory, stdout]);

// Keep the static header in sync with model changes without polling.
Expand Down
14 changes: 7 additions & 7 deletions packages/cli/src/ui/hooks/useGeminiStream.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6074,7 +6074,7 @@ describe('useGeminiStream', () => {
expect(result.current.pendingHistoryItems).toEqual([]);

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

expect(result.current.pendingHistoryItems).toEqual([
Expand Down Expand Up @@ -6134,7 +6134,7 @@ describe('useGeminiStream', () => {
});

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

expect(result.current.pendingHistoryItems).toEqual([]);
Expand All @@ -6146,7 +6146,7 @@ describe('useGeminiStream', () => {
});

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

expect(result.current.pendingHistoryItems).toEqual([
Expand Down Expand Up @@ -6203,7 +6203,7 @@ describe('useGeminiStream', () => {
expect(result.current.pendingHistoryItems).toEqual([]);

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

expect(result.current.pendingHistoryItems).toEqual([
Expand Down Expand Up @@ -6262,7 +6262,7 @@ describe('useGeminiStream', () => {
});

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

const thoughtItems = mockAddItem.mock.calls
Expand Down Expand Up @@ -6794,7 +6794,7 @@ describe('useGeminiStream', () => {
});

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

expect(result.current.pendingHistoryItems).toEqual([]);
Expand All @@ -6807,7 +6807,7 @@ describe('useGeminiStream', () => {
});

await act(async () => {
vi.advanceTimersByTime(60);
vi.advanceTimersByTime(100);
});

expect(result.current.pendingHistoryItems).toEqual([
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/ui/hooks/useGeminiStream.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ const EDIT_TOOL_NAMES = new Set([
ToolNames.WRITE_FILE,
ToolNames.NOTEBOOK_EDIT,
]);
const STREAM_UPDATE_THROTTLE_MS = 60;
const STREAM_UPDATE_THROTTLE_MS = 100;
const STREAM_PENDING_ITEM_MAX_CHARS = 16_384;
// Rows kept in reserve below the commit budget so the incremental commit fires
// BEFORE MarkdownDisplay's safety-net clip (which reserves 2). Keeping the
Expand Down
28 changes: 28 additions & 0 deletions packages/core/src/agents/runtime/agent-core.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ import type {
ContentGenerator,
ContentGeneratorConfig,
} from '../../core/contentGenerator.js';
import { AgentEventEmitter, AgentEventType } from './agent-events.js';
import {
getInvocationContext,
runWithInvocationContext,
Expand Down Expand Up @@ -898,6 +899,33 @@ describe('AgentCore.prepareTools', () => {
});
});

describe('AgentCore STREAM_TEXT batching (#2928)', () => {
it('batches thought and response text separately per chunk', () => {
const emitter = new AgentEventEmitter();
const events: Array<{ text: string; thought: boolean }> = [];
emitter.on(AgentEventType.STREAM_TEXT, (e) => {
events.push({ text: e.text, thought: e.thought ?? false });
});
Comment on lines +903 to +908

@doudouOUC doudouOUC Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] This test creates a bare AgentEventEmitter and calls emit() twice directly — it tests the event emitter's passthrough behavior, not the batching logic added in agent-core.ts:902–936. The production batching (accumulating chunkThoughtText/chunkStreamText across multiple parts in a chunk, emitting once per type) is never exercised.

— Concrete cost: if a future refactor breaks the batching (e.g. reverts to per-part emission, or concatenates thought + response text into one emit), this test still passes. It provides zero regression protection for the change it claims to verify.

Suggested change
it('batches thought and response text separately per chunk', () => {
const emitter = new AgentEventEmitter();
const events: Array<{ text: string; thought: boolean }> = [];
emitter.on(AgentEventType.STREAM_TEXT, (e) => {
events.push({ text: e.text, thought: e.thought ?? false });
});
describe('AgentCore STREAM_TEXT batching (#2928)', () => {
it('coalesces multiple same-type text parts into one emit per chunk', () => {
// Drive AgentCore's streaming loop with a mock ContentGenerator
// that returns a chunk with 2 thought parts + 1 response part,
// then assert exactly 2 STREAM_TEXT events (1 thought, 1 response)
// with concatenated text.
});
});

— qwen3.7-max via Qwen Code /review

emitter.emit(AgentEventType.STREAM_TEXT, {
subagentId: 'test',
round: 1,
text: 'reasoning',
thought: true,
timestamp: Date.now(),
});
emitter.emit(AgentEventType.STREAM_TEXT, {
subagentId: 'test',
round: 1,
text: 'output',
thought: false,
timestamp: Date.now(),
});
expect(events).toHaveLength(2);
expect(events[0].thought).toBe(true);
expect(events[1].thought).toBe(false);
});
});

describe('extractParentToolNames', () => {
const configWithTools = (
tools: Array<{ functionDeclarations?: FunctionDeclaration[] }>,
Expand Down
42 changes: 31 additions & 11 deletions packages/core/src/agents/runtime/agent-core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1001,21 +1001,41 @@ export class AgentCore {
}
const content = resp.candidates?.[0]?.content;
const parts = content?.parts || [];
// #2928: Batch STREAM_TEXT emits per chunk to reduce UI flicker
// during parallel sub-agent execution. Accumulate text parts
// then emit once instead of per-part.
let chunkThoughtText = '';
let chunkStreamText = '';
for (const p of parts) {
const txt = p.text;
const isThought = p.thought ?? false;
if (txt && isThought) roundThoughtText += txt;
if (txt && !isThought) roundText += txt;
if (txt)
this.eventEmitter?.emit(AgentEventType.STREAM_TEXT, {
subagentId: this.subagentId,
runId,
round: turnCounter,
text: txt,
thought: isThought,
timestamp: Date.now(),
});
if (txt && isThought) {
roundThoughtText += txt;
chunkThoughtText += txt;
}
if (txt && !isThought) {
roundText += txt;
chunkStreamText += txt;
}
}
if (chunkThoughtText)

@wenshao wenshao Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The per-chunk batching emits all thought text before all regular text, which reorders vs the original per-part order. For standard reasoning_content providers (always thought-then-text) this is a no-op — but for tagged-thinking models (<think> tags, e.g. the MiniMax provider with taggedThinkingTags: true), TaggedThinkingParser yields interleaved parts like [text, thought, text] within a single chunk ("answer <think>reasoning</think> more answer"). STREAM_TEXT's only live consumer is the web-shell sub-agent view (SubAgentTracker → ACP agent_thought_chunk/agent_message_chunk → appendSubContent, concatenated in arrival order), so reasoning text would render ahead of answer text the model emitted before it, and answer fragments straddling </think>…<think> get merged across the reasoning block. (The CLI is unaffected — it renders sub-agents from committed ROUND_TEXT, not STREAM_TEXT.)

Suggest coalescing only adjacent same-kind runs instead of bucketing by type — flush the running buffer whenever isThought flips — which keeps the flicker win (one emit per contiguous run) without reordering across a type boundary, and still emits exactly thought-then-text for the common path. Please also add a test feeding one chunk with [{text:'A'},{text:'B',thought:true},{text:'C'}] and asserting stream order is preserved — the existing STREAM_TEXT test only feeds thought-first input, so it can't catch this.

— claude-opus-4-8 via Claude Code /qreview

this.eventEmitter?.emit(AgentEventType.STREAM_TEXT, {
subagentId: this.subagentId,
runId,
round: turnCounter,
text: chunkThoughtText,
thought: true,
timestamp: Date.now(),
});
if (chunkStreamText)
this.eventEmitter?.emit(AgentEventType.STREAM_TEXT, {
subagentId: this.subagentId,
runId,
round: turnCounter,
text: chunkStreamText,
thought: false,
timestamp: Date.now(),
});
if (resp.usageMetadata) lastUsage = resp.usageMetadata;

const thoughtSummary = getThoughtSummary(resp);
Expand Down
4 changes: 3 additions & 1 deletion packages/web-shell/client/App.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
useEffect,
useLayoutEffect,
useMemo,
useTransition,
useRef,
useState,
type CSSProperties,
Expand Down Expand Up @@ -5346,6 +5347,7 @@ export function App({
return options;
}, [connection.models]);
const [compactMode, setCompactMode] = useState(false);
const [, startTransition] = useTransition();
const compactModeRef = useRef(compactMode);
compactModeRef.current = compactMode;

Expand Down Expand Up @@ -5428,7 +5430,7 @@ export function App({
const handleToggleCompact = useCallback(() => {
const previous = compactModeRef.current;
const next = !compactModeRef.current;
setCompactMode(next);
startTransition(() => setCompactMode(next));

@wenshao wenshao Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] Wrapping this toggle in startTransition can leave the compact toggle stuck under load. handleToggleCompact is a stable useCallback (deps [reportError, setWorkspaceSetting, t] — compactMode is intentionally excluded) that reads the current value from compactModeRef, which is only refreshed on a committed render (line 1453). Previously setCompactMode(next) was an urgent update, so the ref was current by the next keypress. As a transition (low-priority lane) the compactMode commit can be deferred/starved under exactly the streaming load this PR targets; if a second toggle (Ctrl+O auto-repeat, or a quick double-press) arrives before it commits, compactModeRef.current is still stale, so next = !current recomputes the same value — the UI fails to alternate and setWorkspaceSetting even persists the wrong value.

If the transition is meant to defer the compact re-layout, read the value without the ref — e.g. add compactMode to the callback deps (and drop compactModeRef) so previous/next are always fresh. Otherwise a plain setCompactMode(next) (a single boolean flip is cheap) restores the ref-freshness this stable callback relies on.

— claude-opus-4-8 via Claude Code /qreview

@wenshao wenshao Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] handleToggleCompact derives next from compactModeRef.current, but wrapping setCompactMode(next) in startTransition makes the resulting commit (and the render-time compactModeRef.current = compactMode resync) non-urgent and interruptible. The toggle is a raw keydown handler (Ctrl+O), so holding the key fires this many times/sec via OS key-repeat — and under the heavy streaming this PR targets, transitions are exactly when commits get deferred. Two invocations landing before the transition commits both read the same stale ref, compute the same next, so the toggle stops alternating and issues duplicate setWorkspaceSetting writes (and the .catch rollback can clobber a newer value with a stale previous).

Suggest a functional updater so it never depends on the deferred ref:

startTransition(() => setCompactMode((prev) => !prev));

(compute/persist next inside the updater, or sync compactModeRef.current synchronously before scheduling).

— claude-opus-4-8 via Claude Code /qreview

setWorkspaceSetting('workspace', COMPACT_MODE_SETTING_KEY, next).catch(
(error: unknown) => {
setCompactMode(previous);
Expand Down
Loading