diff --git a/src/browser/components/AIView.tsx b/src/browser/components/AIView.tsx index ae2435bde9..19369c6541 100644 --- a/src/browser/components/AIView.tsx +++ b/src/browser/components/AIView.tsx @@ -44,6 +44,7 @@ import { executeCompaction } from "@/browser/utils/chatCommands"; import { useProviderOptions } from "@/browser/hooks/useProviderOptions"; import { useAutoCompactionSettings } from "../hooks/useAutoCompactionSettings"; import { useSendMessageOptions } from "@/browser/hooks/useSendMessageOptions"; +import { useForceCompaction } from "@/browser/hooks/useForceCompaction"; import { useAPI } from "@/browser/contexts/API"; interface AIViewProps { @@ -139,9 +140,6 @@ const AIViewInner: React.FC = ({ undefined ); - // Track if we've already triggered force compaction for this stream - const forceCompactionTriggeredRef = useRef(null); - // Extract state from workspace state const { messages, canInterrupt, isCompacting, loading, currentModel } = workspaceState; @@ -158,18 +156,8 @@ const AIViewInner: React.FC = ({ // Show warning when: shouldShowWarning flag is true AND not currently compacting const shouldShowCompactionWarning = !isCompacting && autoCompactionResult.shouldShowWarning; - // Force compaction when live usage shows we're about to hit context limit - useEffect(() => { - if ( - !autoCompactionResult.shouldForceCompact || - !canInterrupt || - isCompacting || - forceCompactionTriggeredRef.current === activeStreamMessageId - ) { - return; - } - - forceCompactionTriggeredRef.current = activeStreamMessageId ?? null; + // Handle force compaction callback - memoized to avoid effect re-runs + const handleForceCompaction = useCallback(() => { if (!api) return; void executeCompaction({ api, @@ -177,22 +165,15 @@ const AIViewInner: React.FC = ({ sendMessageOptions: pendingSendOptions, continueMessage: { text: "Continue with the current task" }, }); - }, [ - autoCompactionResult.shouldForceCompact, + }, [api, workspaceId, pendingSendOptions]); + + // Force compaction when live usage shows we're about to hit context limit + useForceCompaction({ + shouldForceCompact: autoCompactionResult.shouldForceCompact, canInterrupt, isCompacting, - activeStreamMessageId, - workspaceId, - pendingSendOptions, - api, - ]); - - // Reset force compaction trigger when stream ends - useEffect(() => { - if (!canInterrupt) { - forceCompactionTriggeredRef.current = null; - } - }, [canInterrupt]); + onTrigger: handleForceCompaction, + }); // Auto-retry state - minimal setter for keybinds and message sent handler // RetryBarrier manages its own state, but we need this for interrupt keybind diff --git a/src/browser/hooks/useForceCompaction.test.ts b/src/browser/hooks/useForceCompaction.test.ts new file mode 100644 index 0000000000..0265c9eca1 --- /dev/null +++ b/src/browser/hooks/useForceCompaction.test.ts @@ -0,0 +1,261 @@ +/** + * Tests for useForceCompaction hook + * + * Verifies the force compaction behavior when usage exceeds the context limit. + * The key invariant: force compaction should only trigger ONCE until usage drops below threshold. + * + * Bug being tested (now fixed): + * When compaction completes and continues with a follow-up message, the guard was reset + * because canInterrupt changed, allowing a second compaction to trigger immediately. + */ + +import { describe, test, expect, mock, beforeEach, afterEach, type Mock } from "bun:test"; +import { renderHook, act, cleanup } from "@testing-library/react"; +import { GlobalWindow } from "happy-dom"; +import { useForceCompaction, type ForceCompactionParams } from "./useForceCompaction"; + +describe("useForceCompaction", () => { + let onTrigger: Mock<() => void>; + + beforeEach(() => { + // Set up DOM environment for React + globalThis.window = new GlobalWindow() as unknown as Window & typeof globalThis; + globalThis.document = globalThis.window.document; + + onTrigger = mock(() => undefined); + }); + + afterEach(() => { + cleanup(); + globalThis.window = undefined as unknown as Window & typeof globalThis; + globalThis.document = undefined as unknown as Document; + }); + + test("triggers compaction when shouldForceCompact and canInterrupt are true", () => { + renderHook(() => + useForceCompaction({ + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }) + ); + + expect(onTrigger).toHaveBeenCalledTimes(1); + }); + + test("does not trigger when shouldForceCompact is false", () => { + renderHook(() => + useForceCompaction({ + shouldForceCompact: false, + canInterrupt: true, + isCompacting: false, + onTrigger, + }) + ); + + expect(onTrigger).not.toHaveBeenCalled(); + }); + + test("does not trigger when canInterrupt is false", () => { + renderHook(() => + useForceCompaction({ + shouldForceCompact: true, + canInterrupt: false, + isCompacting: false, + onTrigger, + }) + ); + + expect(onTrigger).not.toHaveBeenCalled(); + }); + + test("does not trigger when isCompacting is true", () => { + renderHook(() => + useForceCompaction({ + shouldForceCompact: true, + canInterrupt: true, + isCompacting: true, + onTrigger, + }) + ); + + expect(onTrigger).not.toHaveBeenCalled(); + }); + + test("does NOT trigger double compaction when continue message starts after compaction", () => { + // This is the key test for the bug fix! + // + // Timeline being tested: + // 1. Stream running, usage high: shouldForceCompact=true → compaction triggers + // 2. Compaction runs, completes: canInterrupt becomes false briefly + // 3. Continue message starts: canInterrupt=true again, isCompacting=false + // 4. BUG (before fix): second compaction would trigger because guard was reset + // 5. FIX: guard stays set until shouldForceCompact becomes false + + // Phase 1: Initial streaming state exceeding threshold + const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), { + initialProps: { + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }, + }); + + // First compaction should trigger + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Phase 2: Compaction completes, stream ends briefly + act(() => { + rerender({ + shouldForceCompact: true, // Usage still high (compaction summary not yet reflected) + canInterrupt: false, // Stream ended + isCompacting: false, + onTrigger, + }); + }); + + // Still only 1 call (canInterrupt is false) + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Phase 3: Continue message stream starts - THIS IS WHERE THE BUG WOULD OCCUR + act(() => { + rerender({ + shouldForceCompact: true, // Usage still high - would trigger second compaction without fix + canInterrupt: true, // New stream started + isCompacting: false, + onTrigger, + }); + }); + + // Key assertion: still only 1 call, NOT 2! + expect(onTrigger).toHaveBeenCalledTimes(1); + }); + + test("allows new compaction after usage drops below threshold", () => { + // Phase 1: First compaction triggers + const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), { + initialProps: { + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }, + }); + + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Phase 2: Compaction succeeds, usage drops below threshold + act(() => { + rerender({ + shouldForceCompact: false, // Usage dropped! + canInterrupt: false, + isCompacting: false, + onTrigger, + }); + }); + + // Still only 1 call + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Phase 3: Much later, usage builds up again and exceeds threshold + act(() => { + rerender({ + shouldForceCompact: true, // Usage exceeded again + canInterrupt: true, + isCompacting: false, + onTrigger, + }); + }); + + // New compaction should be allowed since usage dropped and rose again + expect(onTrigger).toHaveBeenCalledTimes(2); + }); + + test("does not re-trigger on prop changes while guard is active", () => { + // Ensure multiple re-renders with same conditions don't trigger multiple times + const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), { + initialProps: { + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }, + }); + + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Re-render with same props (simulating React re-render) + act(() => { + rerender({ + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }); + }); + + // Still only 1 call + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Re-render again + act(() => { + rerender({ + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }); + }); + + // Still only 1 call + expect(onTrigger).toHaveBeenCalledTimes(1); + }); + + test("maintains guard across rapid prop changes", () => { + // Simulate rapid prop changes that might occur in React concurrent mode + const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), { + initialProps: { + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, + onTrigger, + }, + }); + + // First trigger + expect(onTrigger).toHaveBeenCalledTimes(1); + + // Rapid transitions simulating complex UI updates + act(() => { + rerender({ + shouldForceCompact: true, + canInterrupt: false, // Brief interruption + isCompacting: false, + onTrigger, + }); + }); + + act(() => { + rerender({ + shouldForceCompact: true, + canInterrupt: true, // Back to interruptible + isCompacting: true, // Now actually compacting + onTrigger, + }); + }); + + act(() => { + rerender({ + shouldForceCompact: true, + canInterrupt: true, + isCompacting: false, // Compaction done + onTrigger, + }); + }); + + // Still only the initial trigger - guard prevented all re-triggers + expect(onTrigger).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/browser/hooks/useForceCompaction.ts b/src/browser/hooks/useForceCompaction.ts new file mode 100644 index 0000000000..d58a312b3f --- /dev/null +++ b/src/browser/hooks/useForceCompaction.ts @@ -0,0 +1,57 @@ +/** + * Force compaction hook - manages automatic compaction when context limit is approached + * + * Triggers compaction when: + * - shouldForceCompact is true (usage exceeds threshold + buffer) + * - canInterrupt is true (there's an active stream to interrupt) + * - isCompacting is false (not already compacting) + * - Haven't already triggered force compaction this cycle + * + * The key invariant: force compaction triggers ONCE per threshold breach. + * The guard only resets when shouldForceCompact becomes false (successful compaction reduced context). + */ + +import { useEffect, useRef } from "react"; + +export interface ForceCompactionParams { + shouldForceCompact: boolean; + canInterrupt: boolean; + isCompacting: boolean; + onTrigger: () => void; +} + +/** + * Hook to manage force compaction triggering + * + * @returns Whether force compaction has been triggered (for testing/debugging) + */ +export function useForceCompaction(params: ForceCompactionParams): boolean { + const { shouldForceCompact, canInterrupt, isCompacting, onTrigger } = params; + + // Track if we've already triggered force compaction (reset when usage drops) + const forceCompactionTriggeredRef = useRef(false); + + // Force compaction when live usage shows we're about to hit context limit + useEffect(() => { + if ( + !shouldForceCompact || + !canInterrupt || + isCompacting || + forceCompactionTriggeredRef.current + ) { + return; + } + + forceCompactionTriggeredRef.current = true; + onTrigger(); + }, [shouldForceCompact, canInterrupt, isCompacting, onTrigger]); + + // Reset force compaction trigger when usage drops below threshold (successful compaction) + useEffect(() => { + if (!shouldForceCompact) { + forceCompactionTriggeredRef.current = false; + } + }, [shouldForceCompact]); + + return forceCompactionTriggeredRef.current; +}