From a5339ddbdffee3c2affef574a4dfd14997134983 Mon Sep 17 00:00:00 2001 From: Ammar Date: Sun, 7 Dec 2025 10:36:44 -0600 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20prevent=20double-compact?= =?UTF-8?q?=20during=20force=20compaction?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The force compaction guard was incorrectly keyed by activeStreamMessageId, which changes when the compaction stream starts. This caused the guard to fail when the continue message stream started after compaction completed, because: 1. Force compaction triggers with activeStreamMessageId='msg-1' 2. Compaction stream starts with new activeStreamMessageId='compact-msg' 3. Compaction completes, stream ends, canInterrupt becomes false 4. Reset effect runs: forceCompactionTriggeredRef = null 5. Continue message stream starts: activeStreamMessageId='continue-msg' 6. Guard check: null !== 'continue-msg' → fails → DOUBLE COMPACT Fix: Use a simple boolean flag that only resets when shouldForceCompact becomes false (i.e., when compaction successfully reduced context usage). This ensures we block all compaction attempts until usage actually drops. Changes: - Extract useForceCompaction hook for clean separation and testability - Use boolean guard instead of message ID tracking - Reset guard only when shouldForceCompact becomes false - Add comprehensive tests verifying: - Basic trigger conditions (shouldForceCompact, canInterrupt, isCompacting) - Double-compact prevention (the main bug fix) - Guard reset after successful compaction - Rapid prop change handling _Generated with mux_ --- src/browser/components/AIView.tsx | 39 +-- src/browser/hooks/useForceCompaction.test.ts | 261 +++++++++++++++++++ src/browser/hooks/useForceCompaction.ts | 57 ++++ 3 files changed, 328 insertions(+), 29 deletions(-) create mode 100644 src/browser/hooks/useForceCompaction.test.ts create mode 100644 src/browser/hooks/useForceCompaction.ts 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; +}