fix(coding-agent): clean up compaction UI lifecycle

executeCompaction added a transcript Spacer(1) the handoff path never adds; it leaked as an orphan blank line on cancelled/failed compaction (only the OK branch cleared it via rebuildChatFromMessages). Removed it, and on the OK branch the loader is stopped + statusContainer cleared before the rebuild so the live loader no longer composites over the reconciled transcript. The finally block stays as the idempotent cancel/fail safety net.
2026-06-14 05:15:52 +09:00
parent eac1d21f93
commit 67cf33ed2d
3 changed files with 128 additions and 1 deletions
@@ -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
@@ -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();
@@ -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<CompactionResult<unknown>> => ({ 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);
+	});
+});