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

Open
aspnmy wants to merge 9 commits into
base: main
Choose a base branch
from
Open

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);

@qwen-code-ci-bot qwen-code-ci-bot Jul 2, 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] refreshStaticRef is never cleaned up on component unmount. Every other timer-ref pattern in this codebase includes a useEffect cleanup (e.g., useDoublePress, useFrameCoalescedFlush, useAnimatedScrollbar). If AppContainer unmounts while a setTimeout(0) is pending, the callback fires against torn-down state — calling stdout.write() and remountStaticHistory() after the Ink instance has been cleaned up.

Suggested change
const refreshStaticRef = useRef<ReturnType<typeof setTimeout> | null>(null);
const refreshStaticRef = useRef<ReturnType<typeof setTimeout> | null>(null);
useEffect(() => {
return () => {
if (refreshStaticRef.current) clearTimeout(refreshStaticRef.current);
};
}, []);

— qwen3.7-max via Qwen Code /review

const refreshStatic = useCallback(() => {
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
if (refreshStaticRef.current) {
clearTimeout(refreshStaticRef.current);
}
remountStaticHistory();
refreshStaticRef.current = setTimeout(() => {

@qwen-code-ci-bot qwen-code-ci-bot Jul 2, 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] The setTimeout(0) debounce breaks the same-commit batching invariant documented in the comment block below (lines 982-999). The onModelChange handler calls refreshStatic() then setCurrentModel(model) synchronously — previously both state updates (setHistoryRemountKey via remountStaticHistory and setCurrentModel) landed in the same React commit via automatic batching. Now refreshStatic defers to a separate macrotask, so setCurrentModel commits first with the OLD historyRemountKey, triggering the exact scenario the comment warns about: "a full-history Static render that bypasses progressive replay (the issue #3899 freeze regression)."

Additionally, the stdout.write(ansiEscapes.clearTerminal) is also deferred — for all non-model-change callers (auto-restore, rewind, Ctrl+O, Alt+T), the terminal clear now happens after React renders the new state, causing a brief visual overlap of old and new static content.

This also breaks 4 existing tests in AppContainer.test.tsx (confirmed: 4 failed | 91 passed):

  • "refreshStatic clears the terminal before remounting history"
  • "auto-restores the just-submitted prompt when cancelling before any meaningful output"
  • "fires refreshStatic in the same handler that updates currentModel"
  • "fires refreshStatic only once per real model change (StrictMode-safe)"
Suggested change
refreshStaticRef.current = setTimeout(() => {
const refreshStatic = useCallback(() => {
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
}
if (refreshStaticRef.current) {
clearTimeout(refreshStaticRef.current);
}
refreshStaticRef.current = setTimeout(() => {
refreshStaticRef.current = null;
remountStaticHistory();
}, 0);
}, [useTerminalBuffer, remountStaticHistory, stdout]);

This keeps clearTerminal synchronous (must run before React renders) while still debouncing remountStaticHistory for coalescing. The model-change path will still need both updates in the same commit — consider also moving setCurrentModel inside the deferred callback, or using queueMicrotask instead of setTimeout(0) so React 18 batches both updates.

— qwen3.7-max via Qwen Code /review

refreshStaticRef.current = null;
if (!useTerminalBuffer) {

@qwen-code-ci-bot qwen-code-ci-bot Jul 7, 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 new ref-based debounce (clearTimeout + setTimeout(0)) introduces a cancellation contract: calling refreshStatic N times in rapid succession should execute remountStaticHistory() exactly once. No existing test covers this coalescing behavior. The closest test (fires refreshStatic only once per real model change) tests a different dedup mechanism (lastNotifiedModelRef), not the setTimeout cancellation.

Consider adding a test with vi.useFakeTimers(): call refreshStatic() twice without advancing timers, then vi.advanceTimersByTime(0), and assert remountStaticHistory ran exactly once.

— qwen3.7-max via Qwen Code /review

@qwen-code-ci-bot qwen-code-ci-bot Jul 10, 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 the entire refreshStatic body (including setHistoryRemountKey inside remountStaticHistory()) to a macrotask. The onModelChange handler (line ~1183) calls refreshStatic() then setCurrentModel(model) synchronously — the comment block at lines 1158-1175 explicitly warns these MUST land in the same React commit or the #3899 freeze regression reoccurs.

This affects 3 call sites: onModelChange, handleCancelAndRewind (line ~2528), and conversation-rewind (line ~3211). All expect synchronous ordering with refreshStatic.

Suggested change
if (!useTerminalBuffer) {
if (!useTerminalBuffer) {
stdout.write(ansiEscapes.clearTerminal);
}
remountStaticHistory();

Keep refreshStatic synchronous for direct callers. If debounce is needed for resize/Ctrl+O paths, extract it into a separate refreshStaticDebounced wrapper used only there.

— qwen3.7-max via Qwen Code /review

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);

@qwen-code-ci-bot qwen-code-ci-bot Jul 7, 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] Two vi.advanceTimersByTime(60) calls were missed when STREAM_UPDATE_THROTTLE_MS was bumped from 60 to 100. While 7 occurrences in this file were correctly updated, the calls at line 4446 and line 4513 (inside the streamContent helper) still advance timers by only 60ms — not enough to flush the 100ms throttle. Both tests fail: line 4446 produces expected 0 to be greater than 0 (pending items empty), and line 4513 produces expected 1 to be 25. This is likely the cause of the failing CI Test check.

Also, the comment at line 7308 still says STREAM_UPDATE_THROTTLE_MS (60ms) — stale after the constant change.

— qwen3.7-max via Qwen Code /review

});

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;

@qwen-code-ci-bot qwen-code-ci-bot Aug 1, 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] Bumping STREAM_UPDATE_THROTTLE_MS 60 → 100 requires updating every vi.advanceTimersByTime(60) in useGeminiStream.test.tsx, but three were missed — lines 6343, 6435, and 6502. Those sites feed tests that flush the buffered stream only via the throttle timer (the stream is held open, so no Finished event triggers an early flush); advancing 60 ms no longer reaches the 100 ms deadline, so flushBufferedStreamEvents() never fires. — Failure scenario: npx vitest run src/ui/hooks/useGeminiStream.test.tsx reports 8 failed | 175 passed (e.g. expected 0 to be greater than 0, expected 1 to be 25). This breakage was reintroduced by the latest merge of main — the prior head had these passing. Fix: change the three remaining vi.advanceTimersByTime(60) calls (6343, 6435, 6502) to (100), or reference STREAM_UPDATE_THROTTLE_MS directly so future bumps don't require hunting through the test file.

— qwen3.8-max-preview via Qwen Code /review

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)', () => {

@qwen-code-ci-bot qwen-code-ci-bot Jul 7, 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 an AgentEventEmitter directly and calls emit() twice, verifying that the emitter dispatches events to listeners. However, the actual batching code lives in agent-core.ts:880-907 — the chunkThoughtText/chunkStreamText accumulation loop inside AgentCore's stream processing. This test never instantiates AgentCore or runs that loop, so it provides false confidence about the batching behavior.

A proper test would feed a multi-part chunk through the actual streaming code path (or extract the batch-and-emit logic into a testable function) and assert that fewer STREAM_TEXT events are emitted than there are parts, with correctly concatenated text.

— qwen3.7-max via Qwen Code /review

it('batches thought and response text separately per chunk', () => {

@qwen-code-ci-bot qwen-code-ci-bot Jul 10, 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] This test creates an AgentEventEmitter directly and calls emit() twice — bypassing the actual batching logic in agent-core.ts:899-937 (the chunkThoughtText/chunkStreamText accumulation loop). The emitter is a passthrough; this test would pass identically with the old per-part code.

A proper test should feed a chunk with multiple text parts through AgentCore and assert that thought and non-thought text are coalesced into exactly 2 events with concatenated text.

— qwen3.7-max via Qwen Code /review

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;

@qwen-code-ci-bot qwen-code-ci-bot Jul 2, 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 batching change has no test coverage — agent-core.test.ts contains no references to STREAM_TEXT or eventEmitter. Two behavioral invariants are untested:

  1. Thought and non-thought text from the same chunk are emitted as separate events (not interleaved).
  2. Multiple text parts of the same type within one chunk are concatenated into a single emission.

A regression (e.g., accidentally emitting per-part again, or mixing thought/non-thought text) would go undetected and could reintroduce the UI flicker this PR aims to fix. Consider adding a test that feeds a mock stream with mixed thought/text parts and asserts on event count, content, and thought flag.

— qwen3.7-max via Qwen Code /review

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