From f18d6ba6e79b59f77821c4145f5f1a7d1eea3469 Mon Sep 17 00:00:00 2001 From: Maciej Krajowski-Kukiel Date: Sat, 1 Aug 2026 15:09:38 +0200 Subject: [PATCH] ctrl c for check run --- bin/pos-cli-check-update-docs.js | 8 +- bin/pos-cli-clone-init.js | 2 +- bin/pos-cli-data-clean.js | 2 +- bin/pos-cli-data-export.js | 2 +- bin/pos-cli-data-import.js | 2 +- bin/pos-cli-data-update.js | 2 +- bin/pos-cli-dns-export.js | 2 +- bin/pos-cli-dns-migrate.js | 2 +- bin/pos-cli-modules-pull.js | 2 +- bin/pos-cli-pull.js | 2 +- bin/pos-cli-uploads-push.js | 2 +- lib/check-messages.js | 13 ++++ lib/check-worker.js | 78 +++++++++++++++++++ lib/check.js | 96 +++++++++++------------- lib/deploy/defaultStrategy.js | 2 +- lib/deploy/directAssetsUploadStrategy.js | 2 +- lib/deploy/dryRunStrategy.js | 2 +- lib/ora.js | 37 +++++++++ lib/spinner.js | 2 +- test/integration/modules-push.test.js | 6 +- test/unit/ora.test.js | 65 ++++++++++++++++ test/utils/spinnerMock.js | 2 +- 22 files changed, 261 insertions(+), 72 deletions(-) create mode 100644 lib/check-messages.js create mode 100644 lib/check-worker.js create mode 100644 lib/ora.js create mode 100644 test/unit/ora.test.js diff --git a/bin/pos-cli-check-update-docs.js b/bin/pos-cli-check-update-docs.js index e09e83de..778db00f 100644 --- a/bin/pos-cli-check-update-docs.js +++ b/bin/pos-cli-check-update-docs.js @@ -1,7 +1,8 @@ #!/usr/bin/env node import { program } from '../lib/program.js'; import logger from '../lib/logger.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; +import { MISSING_PACKAGE_MESSAGE } from '../lib/check-messages.js'; program .name('pos-cli check update-docs') @@ -11,10 +12,7 @@ program try { platformosCheck = await import('@platformos/platformos-check-node'); } catch { - await logger.Error( - 'The @platformos/platformos-check-node package is not installed.\n' + - 'Install it with: npm install @platformos/platformos-check-node' - ); + await logger.Error(MISSING_PACKAGE_MESSAGE); return; } diff --git a/bin/pos-cli-clone-init.js b/bin/pos-cli-clone-init.js index c9b20e74..c0705a86 100755 --- a/bin/pos-cli-clone-init.js +++ b/bin/pos-cli-clone-init.js @@ -5,7 +5,7 @@ import Gateway from '../lib/proxy.js'; import { fetchSettings } from '../lib/settings.js'; import waitForStatus from '../lib/data/waitForStatus.js'; import logger from '../lib/logger.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; program.showHelpAfterError(); program diff --git a/bin/pos-cli-data-clean.js b/bin/pos-cli-data-clean.js index e63bd655..d4402ef7 100755 --- a/bin/pos-cli-data-clean.js +++ b/bin/pos-cli-data-clean.js @@ -7,7 +7,7 @@ import waitForStatus from '../lib/data/waitForStatus.js'; import { fetchSettings } from '../lib/settings.js'; import logger from '../lib/logger.js'; import ServerError from '../lib/ServerError.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; const confirmationText = process.env.CONFIRMATION_TEXT || 'CLEAN DATA'; diff --git a/bin/pos-cli-data-export.js b/bin/pos-cli-data-export.js index 9577862a..c5840407 100755 --- a/bin/pos-cli-data-export.js +++ b/bin/pos-cli-data-export.js @@ -10,7 +10,7 @@ import waitForStatus from '../lib/data/waitForStatus.js'; import downloadFile from '../lib/downloadFile.js'; import logger from '../lib/logger.js'; import report from '../lib/logger/report.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; let gateway; diff --git a/bin/pos-cli-data-import.js b/bin/pos-cli-data-import.js index 20929b5a..46925dec 100755 --- a/bin/pos-cli-data-import.js +++ b/bin/pos-cli-data-import.js @@ -12,7 +12,7 @@ import { uploadFile } from '../lib/s3UploadFile.js'; import { presignUrl } from '../lib/presignUrl.js'; import logger from '../lib/logger.js'; import report from '../lib/logger/report.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; let gateway; diff --git a/bin/pos-cli-data-update.js b/bin/pos-cli-data-update.js index 72a6d86b..5eb47f69 100755 --- a/bin/pos-cli-data-update.js +++ b/bin/pos-cli-data-update.js @@ -8,7 +8,7 @@ import transform from '../lib/data/uploadFiles.js'; import isValidJSON from '../lib/data/isValidJSON.js'; import logger from '../lib/logger.js'; import report from '../lib/logger/report.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; let gateway; diff --git a/bin/pos-cli-dns-export.js b/bin/pos-cli-dns-export.js index 29fe5d67..5b6e7fb8 100755 --- a/bin/pos-cli-dns-export.js +++ b/bin/pos-cli-dns-export.js @@ -2,7 +2,7 @@ import fs from 'fs'; import path from 'path'; -import ora from 'ora'; +import ora from '../lib/ora.js'; import { program } from '../lib/program.js'; import logger from '../lib/logger.js'; diff --git a/bin/pos-cli-dns-migrate.js b/bin/pos-cli-dns-migrate.js index 07c31970..52fb5b12 100755 --- a/bin/pos-cli-dns-migrate.js +++ b/bin/pos-cli-dns-migrate.js @@ -1,6 +1,6 @@ #!/usr/bin/env node -import ora from 'ora'; +import ora from '../lib/ora.js'; import table from 'text-table'; import { program } from '../lib/program.js'; diff --git a/bin/pos-cli-modules-pull.js b/bin/pos-cli-modules-pull.js index f4974219..262dfd52 100755 --- a/bin/pos-cli-modules-pull.js +++ b/bin/pos-cli-modules-pull.js @@ -6,7 +6,7 @@ import logger from '../lib/logger.js'; import { fetchSettings } from '../lib/settings.js'; import downloadFile from '../lib/downloadFile.js'; import waitForStatus from '../lib/data/waitForStatus.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; program .name('pos-cli modules pull') diff --git a/bin/pos-cli-pull.js b/bin/pos-cli-pull.js index 566de982..97949c30 100755 --- a/bin/pos-cli-pull.js +++ b/bin/pos-cli-pull.js @@ -7,7 +7,7 @@ import { fetchSettings } from '../lib/settings.js'; import downloadFile from '../lib/downloadFile.js'; import waitForStatus from '../lib/data/waitForStatus.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; program .name('pos-cli pull') diff --git a/bin/pos-cli-uploads-push.js b/bin/pos-cli-uploads-push.js index 8db28582..832fc521 100755 --- a/bin/pos-cli-uploads-push.js +++ b/bin/pos-cli-uploads-push.js @@ -7,7 +7,7 @@ import { fetchSettings } from '../lib/settings.js'; import { uploadFile } from '../lib/s3UploadFile.js'; import { presignUrl } from '../lib/presignUrl.js'; import logger from '../lib/logger.js'; -import ora from 'ora'; +import ora from '../lib/ora.js'; const uploadZip = async (filepath, gateway) => { diff --git a/lib/check-messages.js b/lib/check-messages.js new file mode 100644 index 00000000..8e976696 --- /dev/null +++ b/lib/check-messages.js @@ -0,0 +1,13 @@ +/** + * Message text shared by everything that loads @platformos/platformos-check-node. + * + * Deliberately a leaf module with no imports: lib/check-worker.js runs on a worker + * thread, which has its own module registry, so importing this from lib/check.js + * instead would re-evaluate that whole graph (ora, chalk, YAML, logger) on the worker + * for a single string — about 100ms on every `pos-cli check run`. + */ +const MISSING_PACKAGE_MESSAGE = + 'The @platformos/platformos-check-node package is not installed.\n' + + 'Install it with: npm install @platformos/platformos-check-node'; + +export { MISSING_PACKAGE_MESSAGE }; diff --git a/lib/check-worker.js b/lib/check-worker.js new file mode 100644 index 00000000..124a8de9 --- /dev/null +++ b/lib/check-worker.js @@ -0,0 +1,78 @@ +import { parentPort, workerData } from 'node:worker_threads'; +import { MISSING_PACKAGE_MESSAGE } from './check-messages.js'; + +/** + * The linter runs here rather than on the main thread so the main thread's event loop + * stays free: that keeps the spinner animating and, more importantly, keeps Ctrl+C + * instant. See lib/ora.js for why a spinner makes Ctrl+C depend on a free event loop. + * + * Nothing here is allowed to exit the process or write to the terminal: the main thread + * owns the spinner and all output, and gets everything through postMessage. + */ + +/** + * A fixable offense carries `fix` / `suggest` closures, and structured clone cannot copy + * functions. Autofix runs on this side for that reason, so only the fields the reporter + * actually prints need to cross back. + */ +const toPlainOffense = (offense) => ({ + check: offense.check, + message: offense.message, + uri: offense.uri, + severity: offense.severity, + start: { line: offense.start.line, character: offense.start.character }, + end: { line: offense.end.line, character: offense.end.character } +}); + +const post = (message) => parentPort.postMessage(message); +const progress = (message) => message && post({ type: 'progress', message }); + +const { path: checkPath, autoFix, checks } = workerData; + +/** + * Wrapped in a function purely so the early exits can `return`. Calling process.exit() + * here instead would risk the last postMessage never reaching the main thread, which + * would turn a clear "unknown check" message into a worker-died error. + */ +const main = async () => { + let platformosCheck; + try { + platformosCheck = await import('@platformos/platformos-check-node'); + } catch { + return post({ type: 'userError', message: MISSING_PACKAGE_MESSAGE }); + } + + if (checks && checks.length > 0) { + const validNames = new Set(platformosCheck.allChecks.map((c) => c.meta.code)); + const unknown = checks.filter((name) => !validNames.has(name)); + if (unknown.length > 0) { + const available = Array.from(validNames).sort().join(', '); + return post({ + type: 'userError', + message: + `Unknown check${unknown.length > 1 ? 's' : ''}: ${unknown.join(', ')}\n` + + `Available checks: ${available}` + }); + } + } + + const result = await platformosCheck.appCheckRun(checkPath, undefined, progress); + + let offenses = checks + ? result.offenses.filter((o) => checks.includes(o.check)) + : result.offenses; + + if (autoFix && offenses.length > 0) { + progress(`Applying automatic fixes to ${offenses.length} offense${offenses.length === 1 ? '' : 's'}...`); + await platformosCheck.autofix(result.app, offenses); + + // Re-run the check after autofix to get updated offenses + progress('Re-checking after fixes...'); + const recheck = await platformosCheck.appCheckRun(checkPath); + offenses = recheck.offenses; + } + + post({ type: 'result', offenses: offenses.map(toPlainOffense) }); +}; + +await main(); diff --git a/lib/check.js b/lib/check.js index a554c7c6..f99ce27a 100644 --- a/lib/check.js +++ b/lib/check.js @@ -1,10 +1,12 @@ import fs from 'fs'; import path from 'path'; +import { Worker } from 'node:worker_threads'; import { fileURLToPath } from 'url'; import logger from './logger.js'; import chalk from 'chalk'; import YAML from 'yaml'; -import ora from 'ora'; +import ora from './ora.js'; +import { MISSING_PACKAGE_MESSAGE } from './check-messages.js'; // Severity levels from platformos-check-node const Severity = { @@ -18,10 +20,7 @@ const loadPlatformosCheck = async () => { const platformosCheck = await import('@platformos/platformos-check-node'); return platformosCheck; } catch { - await logger.Error( - 'The @platformos/platformos-check-node package is not installed.\n' + - 'Install it with: npm install @platformos/platformos-check-node' - ); + await logger.Error(MISSING_PACKAGE_MESSAGE); } }; @@ -349,29 +348,41 @@ const initConfig = async (rootPath) => { } }; +/** + * Marks an error the user is meant to read as-is (an unknown check name, a missing + * package) rather than a crash to be reported with a stack trace. + */ +class UserFacingError extends Error {} + +/** + * Runs the linter on a worker thread and resolves with its plain offenses. + * See lib/check-worker.js for why the check is off the main thread. + */ +const runInWorker = ({ checkPath, autoFix, checks, onProgress }) => + new Promise((resolve, reject) => { + const worker = new Worker(new URL('./check-worker.js', import.meta.url), { + workerData: { path: checkPath, autoFix, checks } + }); + + worker.on('message', (message) => { + if (message.type === 'progress') onProgress(message.message); + else if (message.type === 'result') resolve(message.offenses); + else if (message.type === 'userError') reject(new UserFacingError(message.message)); + }); + worker.on('error', reject); + // A worker that dies without posting anything (an OOM, say) would otherwise leave + // this promise pending forever. Settled promises ignore a later rejection, so this + // is a no-op on the normal path. + worker.on('exit', (code) => reject(new Error(`Check worker stopped with exit code ${code}`))); + }); + const run = async (opts) => { const { path: checkPath, autoFix, checks, format, silent } = opts; await validatePath(checkPath); - const platformosCheck = await loadPlatformosCheck(); - - if (checks && checks.length > 0) { - const validNames = new Set(platformosCheck.allChecks.map((c) => c.meta.code)); - const unknown = checks.filter((name) => !validNames.has(name)); - if (unknown.length > 0) { - const available = Array.from(validNames).sort().join(', '); - await logger.Error( - `Unknown check${unknown.length > 1 ? 's' : ''}: ${unknown.join(', ')}\n` + - `Available checks: ${available}` - ); - return; - } - } - - let offenses = []; + let offenses; let spinner; - let app; // Only show spinner for text output (not JSON) if (format !== 'json' && !silent) { @@ -380,42 +391,25 @@ const run = async (opts) => { } try { - // Run checks with progress callback - const result = await platformosCheck.appCheckRun(checkPath, undefined, (message) => { - if (spinner && message) { - spinner.text = message; + offenses = await runInWorker({ + checkPath, + autoFix, + checks, + onProgress: (message) => { + if (spinner) spinner.text = message; } }); - offenses = checks - ? result.offenses.filter((o) => checks.includes(o.check)) - : result.offenses; - app = result.app; - - // Update spinner with completion info if it's still running - if (spinner && spinner.isSpinning) { - const fileCount = app.length; - spinner.text = `Checked ${fileCount} file${fileCount === 1 ? '' : 's'}`; - } - - if (autoFix && offenses.length > 0) { - if (spinner) { - spinner.text = `Applying automatic fixes to ${offenses.length} offense${offenses.length === 1 ? '' : 's'}...`; - } - await platformosCheck.autofix(app, offenses); - - // Re-run check after autofix to get updated offenses - if (spinner) { - spinner.text = 'Re-checking after fixes...'; - } - const recheck = await platformosCheck.appCheckRun(checkPath); - offenses = recheck.offenses; - } - if (spinner) { spinner.stop(); } } catch (error) { + if (error instanceof UserFacingError) { + if (spinner) spinner.stop(); + await logger.Error(error.message); + return; + } + if (spinner) { spinner.fail('Check failed'); } diff --git a/lib/deploy/defaultStrategy.js b/lib/deploy/defaultStrategy.js index b08cd057..e7683be0 100644 --- a/lib/deploy/defaultStrategy.js +++ b/lib/deploy/defaultStrategy.js @@ -1,4 +1,4 @@ -import ora from 'ora'; +import ora from '../ora.js'; import { makeArchive } from '../archive.js'; import { push, printDeployReport } from '../push.js'; import logger from '../logger.js'; diff --git a/lib/deploy/directAssetsUploadStrategy.js b/lib/deploy/directAssetsUploadStrategy.js index 31f3c5ba..fac2fbfb 100644 --- a/lib/deploy/directAssetsUploadStrategy.js +++ b/lib/deploy/directAssetsUploadStrategy.js @@ -1,5 +1,5 @@ import { performance } from 'perf_hooks'; -import ora from 'ora'; +import ora from '../ora.js'; import Gateway from '../proxy.js'; import { makeArchive } from '../archive.js'; import { deployAssets } from '../assets.js'; diff --git a/lib/deploy/dryRunStrategy.js b/lib/deploy/dryRunStrategy.js index 75b99915..257a8240 100644 --- a/lib/deploy/dryRunStrategy.js +++ b/lib/deploy/dryRunStrategy.js @@ -1,5 +1,5 @@ import { performance } from 'perf_hooks'; -import ora from 'ora'; +import ora from '../ora.js'; import Gateway from '../proxy.js'; import { makeArchive } from '../archive.js'; import { push, printDeployReport } from '../push.js'; diff --git a/lib/ora.js b/lib/ora.js new file mode 100644 index 00000000..543f6f03 --- /dev/null +++ b/lib/ora.js @@ -0,0 +1,37 @@ +import ora from 'ora'; + +/** + * ora, with Ctrl+C left working. + * + * Two ora defaults bear on Ctrl+C. One is turned off here; the other is left on but puts + * a condition on callers. Import this instead of 'ora' anywhere a spinner is shown. + * + * `discardStdin` keeps keys typed during a spinner from being echoed over the spinner + * line, and buys that by putting the terminal into raw mode — which switches off the + * terminal's own ^C -> SIGINT translation. stdin-discarder is supposed to make up for + * that by re-raising SIGINT when it reads a 0x03 byte, but it attaches its reader with + * `prependListener('data')` — which, unlike `on('data')`, does not flip the stream into + * flowing mode — and only calls `resume()` when stdin was already paused, which a + * never-touched `process.stdin` is not. Nothing reads the byte, so Ctrl+C raises no + * signal at all and the terminal does not even echo ^C. + * + * `hideCursor` is left ON, but it comes with a condition worth knowing about. Hiding the + * cursor makes ora register a restore hook via cli-cursor -> restore-cursor -> signal-exit, + * and that hook is a `process.on('SIGINT')` listener. Any JS listener takes SIGINT off + * Node's default disposition: instead of the kernel ending the process outright, the + * signal is queued for the event loop. So Ctrl+C is only as fast as the loop is free, and + * a command that holds the loop in a long synchronous stretch shows ^C and then carries + * on as if nothing happened. Commands should therefore keep the event loop free while a + * spinner is up; the Liquid linter runs on a worker thread for this reason (see + * lib/check-worker.js). Other commands still block under a spinner — `data import` + * parses and transforms the whole file on the main thread, `data export` stringifies and + * writes it — so this is a known-incomplete invariant, not a settled one. The deeper fix + * is either moving that work off the main thread too, or turning hideCursor off here and + * accepting a visible cursor, which removes the SIGINT listener entirely. + * + * The cost of opting out of discardStdin is cosmetic: keys typed during a spinner echo + * over the spinner line. + */ +const spinner = (options = {}) => ora({ discardStdin: false, ...options }); + +export default spinner; diff --git a/lib/spinner.js b/lib/spinner.js index 91719003..62db60f5 100644 --- a/lib/spinner.js +++ b/lib/spinner.js @@ -1,4 +1,4 @@ -import ora from 'ora'; +import ora from './ora.js'; import logger from './logger.js'; /** diff --git a/test/integration/modules-push.test.js b/test/integration/modules-push.test.js index c0f3e47e..b7b63cd4 100644 --- a/test/integration/modules-push.test.js +++ b/test/integration/modules-push.test.js @@ -50,7 +50,11 @@ describe('Server errors', () => { expect(stderr).toMatch('You are unauthorized to do this operation. Check if your Token/URL or email/password are correct.'); }); - test('Wrong version', async () => { + // Needs a `pos_cli_ci_test` module in the Partner Portal with version 0.0.1 already + // published — the "Name has already been taken" error only comes back when that exact + // version exists. The module was deleted from the portal, so the push now stops one + // step earlier with `Module "pos_cli_ci_test" not found`. Unskip once it is recreated. + test.skip('Wrong version', async () => { requireRealCredentials(); const { stdout, stderr } = await run('good', '--email pos-cli-ci@platformos.com'); expect(stdout).toMatch('for access token'); diff --git a/test/unit/ora.test.js b/test/unit/ora.test.js new file mode 100644 index 00000000..66d9e5d3 --- /dev/null +++ b/test/unit/ora.test.js @@ -0,0 +1,65 @@ +/** + * Unit tests for the ora wrapper + * Tests that spinners never put the terminal into raw mode, which is what + * silently disables Ctrl+C for the whole time a spinner is on screen. + */ +import { describe, test, expect, vi, beforeEach } from 'vitest'; +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; +import fg from 'fast-glob'; +import { makeSpinner } from '#test/utils/spinnerMock.js'; + +const oraFactory = vi.fn(() => makeSpinner()); +vi.mock('ora', () => ({ default: (...args) => oraFactory(...args) })); + +const spinner = (await import('#lib/ora.js')).default; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../..'); + +describe('ora wrapper', () => { + beforeEach(() => { + oraFactory.mockClear(); + }); + + // Exact match rather than objectContaining: it pins the caller options through *and* + // proves nothing else is injected — notably that hideCursor is left at ora's default. + test('disables discardStdin alongside the caller options', () => { + spinner({ text: 'Exporting', stream: process.stdout, interval: 500 }); + + expect(oraFactory).toHaveBeenCalledWith({ + discardStdin: false, + text: 'Exporting', + stream: process.stdout, + interval: 500 + }); + }); + + test('works with no options', () => { + spinner(); + + expect(oraFactory).toHaveBeenCalledWith({ discardStdin: false }); + }); + + test('lets a caller opt back into discarding stdin', () => { + spinner({ text: 'Prompting', discardStdin: true }); + + expect(oraFactory).toHaveBeenCalledWith(expect.objectContaining({ discardStdin: true })); + }); + + // The wrapper only helps where it is actually used, so guard the import itself: + // a command that reaches for 'ora' directly gets the raw-mode default back and + // becomes impossible to interrupt. + test('no command imports ora directly', async () => { + const files = await fg(['bin/**/*.js', 'lib/**/*.js', 'mcp-min/**/*.js', 'scripts/**/*.js'], { + cwd: repoRoot, + ignore: ['**/node_modules/**', 'lib/ora.js'] + }); + + const offenders = files.filter(file => + /^\s*import\s+.*\sfrom\s+['"]ora['"]/m.test(fs.readFileSync(path.join(repoRoot, file), 'utf8')) + ); + + expect(offenders, `import from '#lib/ora.js' instead in: ${offenders.join(', ')}`).toEqual([]); + }); +}); diff --git a/test/utils/spinnerMock.js b/test/utils/spinnerMock.js index 054c5b54..31321027 100644 --- a/test/utils/spinnerMock.js +++ b/test/utils/spinnerMock.js @@ -1,6 +1,6 @@ import { vi } from 'vitest'; /** Returns a fresh spinner mock with all ora-compatible methods stubbed. */ -const makeSpinner = () => ({ start: vi.fn(), succeed: vi.fn(), fail: vi.fn(), warn: vi.fn() }); +const makeSpinner = () => ({ start: vi.fn(), stop: vi.fn(), succeed: vi.fn(), fail: vi.fn(), warn: vi.fn() }); export { makeSpinner };