mirror of
https://github.com/OpenRouterTeam/spawn.git
synced 2026-04-28 03:49:31 +00:00
fix: remove spinner from delete to prevent output overlap (#2487)
* fix: remove spinner from delete command to prevent output overlap The delete spinner in confirmAndDelete collided with cloud-specific destroy functions that print their own progress (logStep/logInfo). This caused the "Instance destroyed" message to overwrite the spinner line without a newline, producing garbled output. Remove the spinner and let the cloud destroy functions handle progress output directly, then show a clean success/failure message after. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: redirect cloud destroy output into delete spinner Cloud destroy functions (logStep/logInfo) write progress to stderr, which collided with the @clack spinner on the terminal. Now stderr writes during the delete are intercepted and fed into s.message() so the spinner text updates in place instead of garbling the output. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * test: add delete spinner behavior tests Verify that confirmAndDelete: - Feeds stderr output from cloud destroy functions into spinner.message() - Calls spinner.clear() (not stop) so no spinner chrome remains - Shows p.log.success with the last stderr message as detail - Shows p.log.error on failure - Always restores process.stderr.write, even on error - Works when destroy produces no stderr output Also adds spinnerClear to the shared test-helpers mock. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove global cloud module mocks that polluted other tests Only mock hetzner (the cloud used by test records). Other cloud modules are left un-mocked since they're never called for hetzner records. This fixes the DO payment warning test failures caused by mock.module being process-global in Bun. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: lab <6723574+louisgv@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
4318acad19
commit
6439cba58c
4 changed files with 216 additions and 4 deletions
|
|
@ -1,6 +1,6 @@
|
|||
{
|
||||
"name": "@openrouter/spawn",
|
||||
"version": "0.16.7",
|
||||
"version": "0.16.8",
|
||||
"type": "module",
|
||||
"bin": {
|
||||
"spawn": "cli.js"
|
||||
|
|
|
|||
185
packages/cli/src/__tests__/delete-spinner.test.ts
Normal file
185
packages/cli/src/__tests__/delete-spinner.test.ts
Normal file
|
|
@ -0,0 +1,185 @@
|
|||
/**
|
||||
* delete-spinner.test.ts — Tests that confirmAndDelete feeds cloud destroy
|
||||
* stderr output into the spinner message, then clears the spinner and shows
|
||||
* the final result via p.log.success/error with the last stderr message.
|
||||
*/
|
||||
|
||||
import { afterEach, beforeEach, describe, expect, it, mock } from "bun:test";
|
||||
import { mkdirSync, rmSync } from "node:fs";
|
||||
import { join } from "node:path";
|
||||
import { mockClackPrompts } from "./test-helpers.js";
|
||||
|
||||
// ── Mock @clack/prompts (must be before importing the module under test) ──
|
||||
const clack = mockClackPrompts({
|
||||
confirm: mock(async () => true),
|
||||
});
|
||||
|
||||
// ── Mock only hetzner (the cloud used by test records) ──────────────────
|
||||
// Other cloud modules are left un-mocked to avoid process-global pollution
|
||||
// (mock.module is global in Bun and would break other test files).
|
||||
// They import fine but are never called since test records use cloud: "hetzner".
|
||||
const mockHetznerDestroy = mock(() => Promise.resolve());
|
||||
mock.module("../hetzner/hetzner.js", () => ({
|
||||
ensureHcloudToken: mock(() => Promise.resolve()),
|
||||
destroyServer: mockHetznerDestroy,
|
||||
}));
|
||||
|
||||
// History uses real module — SPAWN_HOME is pointed at a temp dir in beforeEach
|
||||
|
||||
// ── Import the module under test ──────────────────────────────────────────
|
||||
const { confirmAndDelete } = await import("../commands/delete.js");
|
||||
|
||||
// ── Helpers ───────────────────────────────────────────────────────────────
|
||||
|
||||
function makeRecord(cloud: string, serverName: string) {
|
||||
return {
|
||||
id: "test-id",
|
||||
agent: "claude",
|
||||
cloud,
|
||||
timestamp: new Date().toISOString(),
|
||||
connection: {
|
||||
ip: "10.0.0.1",
|
||||
user: "root",
|
||||
server_name: serverName,
|
||||
cloud,
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
// ── Tests ─────────────────────────────────────────────────────────────────
|
||||
|
||||
describe("confirmAndDelete spinner behavior", () => {
|
||||
let testDir: string;
|
||||
let savedSpawnHome: string | undefined;
|
||||
|
||||
beforeEach(() => {
|
||||
testDir = join(process.env.HOME ?? "", `.spawn-test-delete-${Date.now()}`);
|
||||
mkdirSync(testDir, {
|
||||
recursive: true,
|
||||
});
|
||||
savedSpawnHome = process.env.SPAWN_HOME;
|
||||
process.env.SPAWN_HOME = testDir;
|
||||
|
||||
clack.confirm.mockImplementation(async () => true);
|
||||
clack.spinnerStart.mockClear();
|
||||
clack.spinnerStop.mockClear();
|
||||
clack.spinnerMessage.mockClear();
|
||||
clack.spinnerClear.mockClear();
|
||||
clack.logSuccess.mockClear();
|
||||
clack.logError.mockClear();
|
||||
mockHetznerDestroy.mockClear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
if (savedSpawnHome !== undefined) {
|
||||
process.env.SPAWN_HOME = savedSpawnHome;
|
||||
} else {
|
||||
delete process.env.SPAWN_HOME;
|
||||
}
|
||||
rmSync(testDir, {
|
||||
recursive: true,
|
||||
force: true,
|
||||
});
|
||||
});
|
||||
|
||||
it("feeds stderr output from destroy into spinner.message()", async () => {
|
||||
// Simulate a cloud destroy function that writes progress to stderr
|
||||
mockHetznerDestroy.mockImplementation(async () => {
|
||||
process.stderr.write("\x1b[36mDestroying Hetzner server srv-123...\x1b[0m\n");
|
||||
process.stderr.write("\x1b[32mServer srv-123 destroyed\x1b[0m\n");
|
||||
});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
const result = await confirmAndDelete(record, null);
|
||||
|
||||
expect(result).toBe(true);
|
||||
|
||||
// Spinner should have received stripped (no ANSI) messages
|
||||
const messageCalls = clack.spinnerMessage.mock.calls.map((c: unknown[]) => c[0]);
|
||||
expect(messageCalls).toContain("Destroying Hetzner server srv-123...");
|
||||
expect(messageCalls).toContain("Server srv-123 destroyed");
|
||||
});
|
||||
|
||||
it("calls spinner.clear() instead of spinner.stop()", async () => {
|
||||
mockHetznerDestroy.mockImplementation(async () => {
|
||||
process.stderr.write("Server srv-123 destroyed\n");
|
||||
});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
await confirmAndDelete(record, null);
|
||||
|
||||
expect(clack.spinnerClear).toHaveBeenCalledTimes(1);
|
||||
expect(clack.spinnerStop).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("shows success with last stderr message as detail", async () => {
|
||||
mockHetznerDestroy.mockImplementation(async () => {
|
||||
process.stderr.write("Destroying Hetzner server srv-123...\n");
|
||||
process.stderr.write("Server srv-123 destroyed\n");
|
||||
});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
await confirmAndDelete(record, null);
|
||||
|
||||
expect(clack.logSuccess).toHaveBeenCalledTimes(1);
|
||||
const msg = clack.logSuccess.mock.calls[0][0];
|
||||
expect(msg).toContain('Server "srv-123" deleted');
|
||||
expect(msg).toContain("Server srv-123 destroyed");
|
||||
});
|
||||
|
||||
it("shows error with detail on delete failure", async () => {
|
||||
mockHetznerDestroy.mockImplementation(async () => {
|
||||
process.stderr.write("Connection refused\n");
|
||||
throw new Error("API timeout");
|
||||
});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
const result = await confirmAndDelete(record, null);
|
||||
|
||||
expect(result).toBe(false);
|
||||
expect(clack.spinnerClear).toHaveBeenCalledTimes(1);
|
||||
expect(clack.logError).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("restores process.stderr.write after delete", async () => {
|
||||
const origWrite = process.stderr.write;
|
||||
|
||||
mockHetznerDestroy.mockImplementation(async () => {
|
||||
process.stderr.write("done\n");
|
||||
});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
await confirmAndDelete(record, null);
|
||||
|
||||
expect(process.stderr.write).toBe(origWrite);
|
||||
});
|
||||
|
||||
it("restores process.stderr.write even on error", async () => {
|
||||
const origWrite = process.stderr.write;
|
||||
|
||||
mockHetznerDestroy.mockImplementation(async () => {
|
||||
process.stderr.write("boom\n");
|
||||
throw new Error("kaboom");
|
||||
});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
await confirmAndDelete(record, null);
|
||||
|
||||
expect(process.stderr.write).toBe(origWrite);
|
||||
});
|
||||
|
||||
it("works with no stderr output from destroy", async () => {
|
||||
// Destroy succeeds silently
|
||||
mockHetznerDestroy.mockImplementation(async () => {});
|
||||
|
||||
const record = makeRecord("hetzner", "srv-123");
|
||||
const result = await confirmAndDelete(record, null);
|
||||
|
||||
expect(result).toBe(true);
|
||||
expect(clack.spinnerClear).toHaveBeenCalledTimes(1);
|
||||
expect(clack.logSuccess).toHaveBeenCalledTimes(1);
|
||||
// No detail suffix when no stderr output
|
||||
const msg = clack.logSuccess.mock.calls[0][0];
|
||||
expect(msg).toBe('Server "srv-123" deleted');
|
||||
});
|
||||
});
|
||||
|
|
@ -102,6 +102,7 @@ export interface ClackPromptsMock {
|
|||
spinnerStart: ReturnType<typeof mock>;
|
||||
spinnerStop: ReturnType<typeof mock>;
|
||||
spinnerMessage: ReturnType<typeof mock>;
|
||||
spinnerClear: ReturnType<typeof mock>;
|
||||
intro: ReturnType<typeof mock>;
|
||||
outro: ReturnType<typeof mock>;
|
||||
cancel: ReturnType<typeof mock>;
|
||||
|
|
@ -132,6 +133,7 @@ export function mockClackPrompts(overrides?: Partial<ClackPromptsMock>): ClackPr
|
|||
spinnerStart: mock(() => {}),
|
||||
spinnerStop: mock(() => {}),
|
||||
spinnerMessage: mock(() => {}),
|
||||
spinnerClear: mock(() => {}),
|
||||
intro: mock(() => {}),
|
||||
outro: mock(() => {}),
|
||||
cancel: mock(() => {}),
|
||||
|
|
@ -149,6 +151,7 @@ export function mockClackPrompts(overrides?: Partial<ClackPromptsMock>): ClackPr
|
|||
start: mocks.spinnerStart,
|
||||
stop: mocks.spinnerStop,
|
||||
message: mocks.spinnerMessage,
|
||||
clear: mocks.spinnerClear,
|
||||
}),
|
||||
log: {
|
||||
step: mocks.logStep,
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ import { loadManifest } from "../manifest.js";
|
|||
import { validateMetadataValue, validateServerIdentifier } from "../security.js";
|
||||
import { getHistoryPath } from "../shared/paths.js";
|
||||
import { asyncTryCatch, asyncTryCatchIf, isNetworkError, tryCatch } from "../shared/result.js";
|
||||
import { isString } from "../shared/type-guards.js";
|
||||
import { ensureSpriteAuthenticated, ensureSpriteCli, destroyServer as spriteDestroyServer } from "../sprite/sprite.js";
|
||||
import { activeServerPicker, resolveListFilters } from "./list.js";
|
||||
import { getErrorMessage, isInteractiveTTY } from "./shared.js";
|
||||
|
|
@ -195,12 +196,35 @@ export async function confirmAndDelete(record: SpawnRecord, manifest: Manifest |
|
|||
const s = p.spinner();
|
||||
s.start(`Deleting ${label}...`);
|
||||
|
||||
const success = await execDeleteServer(record);
|
||||
// Cloud destroy functions log progress to stderr (logStep/logInfo).
|
||||
// Redirect those writes into s.message() so the spinner text updates
|
||||
// in place, then clear the spinner and replay the final message as a
|
||||
// normal log line so no spinner chrome remains in the terminal.
|
||||
const origStderrWrite = process.stderr.write;
|
||||
const ANSI_RE = /\x1b\[[0-9;]*m/g;
|
||||
let lastMessage = "";
|
||||
process.stderr.write = function stderrToSpinner(chunk: string | Uint8Array) {
|
||||
const text = isString(chunk) ? chunk : "";
|
||||
const stripped = text.replace(ANSI_RE, "").trim();
|
||||
if (stripped) {
|
||||
lastMessage = stripped;
|
||||
s.message(stripped);
|
||||
}
|
||||
return true;
|
||||
};
|
||||
|
||||
const deleteResult = await asyncTryCatch(() => execDeleteServer(record));
|
||||
process.stderr.write = origStderrWrite;
|
||||
|
||||
const success = deleteResult.ok ? deleteResult.data : false;
|
||||
|
||||
s.clear();
|
||||
if (success) {
|
||||
s.stop(`Server "${label}" deleted.`);
|
||||
const detail = lastMessage ? `: ${lastMessage}` : "";
|
||||
p.log.success(`Server "${label}" deleted${detail}`);
|
||||
} else {
|
||||
s.stop("Delete failed.");
|
||||
const detail = lastMessage ? `: ${lastMessage}` : "";
|
||||
p.log.error(`Failed to delete "${label}"${detail}`);
|
||||
}
|
||||
return success;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue