diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 7fc96cb5e..e8435be95 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,6 +1,9 @@ # Changelog ## [Unreleased] +### Fixed + +- Fixed the compaction flow (`/compact` and plan-mode "Approve and compact context") leaving UI artifacts. `executeCompaction` added a `Spacer(1)` to the transcript that the sibling handoff path never adds and that leaked as an orphan blank line whenever compaction was cancelled or failed; that spacer is removed. On success the compaction loader is now stopped and the status container cleared *before* the transcript is rebuilt, so the live loader row no longer flickers over the reconciled transcript near the status seam (the idempotent `finally` still covers the cancel/fail paths) ([#2486](https://github.com/can1357/oh-my-pi/issues/2486)) ## [15.12.5] - 2026-06-13 ### Changed diff --git a/packages/coding-agent/src/modes/controllers/command-controller.ts b/packages/coding-agent/src/modes/controllers/command-controller.ts index 8d10d01fd..fcda2b5e5 100644 --- a/packages/coding-agent/src/modes/controllers/command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/command-controller.ts @@ -1026,7 +1026,6 @@ export class CommandController { } this.ctx.statusContainer.clear(); - this.ctx.chatContainer.addChild(new Spacer(1)); const label = isAuto ? "Auto-compacting context... (esc to cancel)" : "Compacting context... (esc to cancel)"; const compactingLoader = new Loader( this.ctx.ui, @@ -1047,6 +1046,8 @@ export class CommandController { : undefined; await this.ctx.session.compact(instructions, options); + compactingLoader.stop(); + this.ctx.statusContainer.clear(); this.ctx.rebuildChatFromMessages(); this.ctx.statusLine.invalidate(); diff --git a/packages/coding-agent/test/compaction-lifecycle.test.ts b/packages/coding-agent/test/compaction-lifecycle.test.ts new file mode 100644 index 000000000..b6275266d --- /dev/null +++ b/packages/coding-agent/test/compaction-lifecycle.test.ts @@ -0,0 +1,123 @@ +import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "bun:test"; +import { CompactionCancelledError, type CompactionResult } from "@oh-my-pi/pi-agent-core/compaction"; +import { CommandController } from "@oh-my-pi/pi-coding-agent/modes/controllers/command-controller"; +import { getThemeByName, setThemeInstance, type Theme, theme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types"; +import { Container, Spacer } from "@oh-my-pi/pi-tui"; + +/** + * Contract under test: `CommandController.executeCompaction` must not leak + * transient UI across either terminal state. + * + * - A cancelled compaction (session.compact rejects with the real + * CompactionCancelledError the code branches on) must leave the chat + * transcript byte-for-byte as it was — no orphan Spacer pushed into + * chatContainer — and must drain the status container's loader. + * - A successful compaction must drain the status container's loader once it + * resolves. + * + * Exercised only through the public `executeCompaction` entrypoint with real + * in-memory Container instances and a session stub whose `compact()` outcome we + * drive. + */ +function buildCtx(compact: InteractiveModeContext["session"]["compact"]) { + const chatContainer = new Container(); + const statusContainer = new Container(); + // Pre-existing transcript content. The regression we defend leaked an extra + // Spacer into this container on the cancel path, so we seed it with real + // children and require the count to survive the call untouched. + chatContainer.addChild(new Spacer(1)); + chatContainer.addChild(new Spacer(1)); + + // Record the status container's state at the instant the transcript rebuild + // runs, so a test can prove cleanup happens BEFORE the rebuild (the fix) and + // not merely in the finally that runs after it. + let statusChildrenAtRebuild: number | undefined; + const rebuildChatFromMessages = vi.fn(() => { + statusChildrenAtRebuild = statusContainer.children.length; + }); + const showError = vi.fn(); + const ctx = { + loadingAnimation: undefined, + chatContainer, + statusContainer, + ui: { requestRender: vi.fn(), requestComponentRender: vi.fn() }, + session: { compact }, + rebuildChatFromMessages, + statusLine: { invalidate: vi.fn() }, + updateEditorTopBorder: vi.fn(), + showError, + flushCompactionQueue: vi.fn(async () => undefined), + } as unknown as InteractiveModeContext; + + return { + ctx, + chatContainer, + statusContainer, + rebuildChatFromMessages, + showError, + statusAtRebuild: () => statusChildrenAtRebuild, + }; +} + +describe("executeCompaction UI lifecycle", () => { + let priorTheme: Theme | undefined; + + beforeAll(async () => { + // The compacting Loader colorizes through the active theme on construction. + // Capture the prior global theme first so afterAll can restore it and not + // couple later suites sharing this process to our dark override. + priorTheme = theme; + const dark = await getThemeByName("dark"); + if (!dark) throw new Error("Expected dark theme"); + setThemeInstance(dark); + }); + + afterAll(() => { + if (priorTheme) setThemeInstance(priorTheme); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("leaves the transcript untouched and drains the loader when compaction is cancelled", async () => { + const compact = vi.fn(async () => { + throw new CompactionCancelledError(); + }); + const { ctx, chatContainer, statusContainer, rebuildChatFromMessages, showError } = buildCtx(compact); + const childrenBefore = chatContainer.children.length; + + const controller = new CommandController(ctx); + const outcome = await controller.executeCompaction(); + + expect(outcome).toBe("cancelled"); + // No orphan Spacer leaked into the chat transcript on the cancel path. + expect(chatContainer.children).toHaveLength(childrenBefore); + // The compacting loader was removed from the status container. + expect(statusContainer.children).toHaveLength(0); + // Proof the cancel branch ran instead of the success branch. + expect(showError).toHaveBeenCalledWith("Compaction cancelled"); + expect(rebuildChatFromMessages).not.toHaveBeenCalled(); + }); + + it("drains the loader after a successful compaction resolves", async () => { + const compact = vi.fn( + async (): Promise> => ({ summary: "", firstKeptEntryId: "", tokensBefore: 0 }), + ); + const { ctx, statusContainer, rebuildChatFromMessages, statusAtRebuild } = buildCtx(compact); + + const controller = new CommandController(ctx); + const outcome = await controller.executeCompaction(); + + expect(outcome).toBe("ok"); + // Status container is empty once compaction resolves. + expect(statusContainer.children).toHaveLength(0); + // Proof the success branch ran (rebuild happens only on the ok path). + expect(rebuildChatFromMessages).toHaveBeenCalledTimes(1); + // The loader was drained BEFORE the transcript rebuild, not only by the + // finally that runs afterward: the status container was already empty at + // the instant rebuildChatFromMessages ran (1 leaked loader without the fix). + expect(statusAtRebuild()).toBe(0); + }); +});