Skip to content

Speculative fixes to try to fix react error. #19508

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

Merged
jacob314 merged 1 commit into from
Feb 19, 2026
+35 −18
Merged

Speculative fixes to try to fix react error. #19508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 12 additions & 8 deletions packages/cli/src/ui/components/messages/ToolShared.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,20 +80,24 @@ export function useFocusHint(
isThisShellFocused: boolean,
resultDisplay: ToolResultDisplay | undefined,
) {
const [lastUpdateTime, setLastUpdateTime] = useState<Date | null>(null);
const [userHasFocused, setUserHasFocused] = useState(false);

// Derive a stable reset key for the inactivity timer. For strings and arrays
// (shell output), we use the length to capture updates without referential
// identity issues or expensive deep comparisons.
const resetKey =
typeof resultDisplay === 'string'
? resultDisplay.length
: Array.isArray(resultDisplay)
? resultDisplay.length
: !!resultDisplay;

const showFocusHint = useInactivityTimer(
isThisShellFocusable,
lastUpdateTime ? lastUpdateTime.getTime() : 0,
resetKey,
SHELL_FOCUS_HINT_DELAY_MS,
);

useEffect(() => {
Copy link
Contributor Author

@jacob314 jacob314 Feb 19, 2026

Choose a reason for hiding this comment

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

Fyi @galz10 I'm a bit at a lost why we were using lastupdateTime as a key here. I think that was renerally a bug. Was the intent we should only delay showing the focus hint after the content of a tool actually changed?

As is this code would trigger an unneeded setState with every change to resultDisplay perhaps doubling the number of updates when there is a react update storm.

if (resultDisplay) {
setLastUpdateTime(new Date());
}
}, [resultDisplay]);

useEffect(() => {
if (isThisShellFocused) {
setUserHasFocused(true);
Expand Down
8 changes: 4 additions & 4 deletions packages/cli/src/ui/hooks/useConsoleMessages.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ describe('useConsoleMessages', () => {
});

await act(async () => {
await vi.advanceTimersByTimeAsync(20);
await vi.advanceTimersByTimeAsync(60);
});

expect(result.current.consoleMessages).toEqual([
Expand All @@ -114,7 +114,7 @@ describe('useConsoleMessages', () => {
});

await act(async () => {
await vi.advanceTimersByTimeAsync(20);
await vi.advanceTimersByTimeAsync(60);
});

expect(result.current.consoleMessages).toEqual([
Expand All @@ -131,7 +131,7 @@ describe('useConsoleMessages', () => {
});

await act(async () => {
await vi.advanceTimersByTimeAsync(20);
await vi.advanceTimersByTimeAsync(60);
});

expect(result.current.consoleMessages).toEqual([
Expand All @@ -148,7 +148,7 @@ describe('useConsoleMessages', () => {
});

await act(async () => {
await vi.advanceTimersByTimeAsync(20);
await vi.advanceTimersByTimeAsync(60);
});

expect(result.current.consoleMessages).toHaveLength(1);
Expand Down
25 changes: 19 additions & 6 deletions packages/cli/src/ui/hooks/useConsoleMessages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
useEffect,
useReducer,
useRef,
useTransition,
startTransition,
} from 'react';
import type { ConsoleMessageItem } from '../types.js';
import {
Expand Down Expand Up @@ -71,10 +71,11 @@ export function useConsoleMessages(): UseConsoleMessagesReturn {
const [consoleMessages, dispatch] = useReducer(consoleMessagesReducer, []);
const messageQueueRef = useRef<ConsoleMessageItem[]>([]);
const timeoutRef = useRef<NodeJS.Timeout | null>(null);
const [, startTransition] = useTransition();
const isProcessingRef = useRef(false);

const processQueue = useCallback(() => {
if (messageQueueRef.current.length > 0) {
isProcessingRef.current = true;
const messagesToProcess = messageQueueRef.current;
messageQueueRef.current = [];
startTransition(() => {
Expand All @@ -87,15 +88,26 @@ export function useConsoleMessages(): UseConsoleMessagesReturn {
const handleNewMessage = useCallback(
(message: ConsoleMessageItem) => {
messageQueueRef.current.push(message);
if (!timeoutRef.current) {
// Batch updates using a timeout. 16ms is a reasonable delay to batch
// rapid-fire messages without noticeable lag.
timeoutRef.current = setTimeout(processQueue, 16);
if (!isProcessingRef.current && !timeoutRef.current) {
// Batch updates using a timeout. 50ms is a reasonable delay to batch
// rapid-fire messages without noticeable lag while avoiding React update
// queue flooding.
timeoutRef.current = setTimeout(processQueue, 50);
}
},
[processQueue],
);

// Once the updated consoleMessages have been committed to the screen,
// we can safely process the next batch of queued messages if any exist.
// This completely eliminates overlapping concurrent updates to this state.
useEffect(() => {
isProcessingRef.current = false;
if (messageQueueRef.current.length > 0 && !timeoutRef.current) {
timeoutRef.current = setTimeout(processQueue, 50);
}
}, [consoleMessages, processQueue]);

useEffect(() => {
const handleConsoleLog = (payload: ConsoleLogPayload) => {
let content = payload.content;
Expand Down Expand Up @@ -149,6 +161,7 @@ export function useConsoleMessages(): UseConsoleMessagesReturn {
timeoutRef.current = null;
}
messageQueueRef.current = [];
isProcessingRef.current = true;
startTransition(() => {
dispatch({ type: 'CLEAR' });
});
Expand Down
Loading