diff --git a/openspec/changes/cli-loading-state/design.md b/openspec/changes/cli-loading-state/design.md new file mode 100644 index 0000000..da390f5 --- /dev/null +++ b/openspec/changes/cli-loading-state/design.md @@ -0,0 +1,327 @@ +# Design — cli-loading-state + +## Overview + +A new CLI presentation utility, `src/cli/loading.ts`, exports `withLoading(stream, fn, options?)` — a higher-order async wrapper that renders a 10-frame Braille spinner to a `NodeJS.WriteStream` (always stderr in practice) on an interval while `fn` runs, then erases the spinner line in a `finally` block before returning whatever `fn` produced. The wrapper is transparent to the caller's `Result` (R12) and never throws (R1) — if `fn` rejects, cleanup still runs and the rejection propagates unchanged. `run.ts` and `vod.ts` each gain one wrapped await; no other production code paths change. + +## Module Layout + +``` +src/cli/ + ansi.ts # existing — isColorEnabled, accent/dim/error/muted helpers + loading.ts # NEW — withLoading, isSpinnerEnabled, SPINNER_FRAMES + loading.test.ts # NEW — unit tests + render.ts # existing — renderPassage / renderParseError / renderRepoError + run.ts # MODIFY — wrap the getPassage await + vod.ts # MODIFY — wrap the getPassage await +tests/ + vod-smoke.test.ts # existing — extended with one assertion (preferred over new file) +``` + +`src/cli/loading.ts` exports: + +- `withLoading(stream, fn, options?): Promise` — the wrapper +- `isSpinnerEnabled(stream): boolean` — the TTY/env gate +- `SPINNER_FRAMES` — readonly 10-tuple of Braille glyphs (`as const`) + +Nothing else is exported. The interval default (`80`) is an internal default inside `withLoading`, not a named export — callers should not need to know it. + +## Types and Signatures + +Verbatim TypeScript for `src/cli/loading.ts`: + +```ts +// src/cli/loading.ts — animated stderr spinner for blocking CLI awaits. +// Gated on per-stream TTY + NO_COLOR/FORCE_COLOR (mirrors isColorEnabled). +// R1/R12 spirit: transparent to the Result that fn returns; never throws. +// R3 N/A: presentation utility, not a port. R2/R11: plain function, no class, +// no decorators. fn is a thunk, not an event handler. + +export const SPINNER_FRAMES = [ + "⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏", +] as const; + +export type SpinnerFrame = (typeof SPINNER_FRAMES)[number]; + +export type WithLoadingOptions = { + readonly interval?: number; // ms between frame writes; default 80 +}; + +export function isSpinnerEnabled(stream: NodeJS.WriteStream): boolean { /* ... */ } + +export function withLoading( + stream: NodeJS.WriteStream, + fn: () => Promise, + options?: WithLoadingOptions, +): Promise { /* ... */ } +``` + +Notes on the type choices: + +- `SpinnerFrame` is exported only because it falls out of `as const` for free — it is not consumed anywhere outside the module. Leaving it exported costs nothing and lets future tests assert frame identity. This is NOT a template-literal type (R7 N/A here anyway — presentation layer). +- `WithLoadingOptions` is `readonly` with a single optional field. No conditional/mapped types (R7). +- `Promise` is the return — `withLoading` is transparent to `T`. When `T` is `Result` (the actual production usage), the helper passes it through byte-identically (R12). +- The interval handle type is `ReturnType` — internal, not exported. Node typings call this `NodeJS.Timeout`; using `ReturnType` avoids importing the `NodeJS` global into the helper just for one local variable type. + +## Control Flow + +### `isSpinnerEnabled(stream)` + +Implementation mirrors `isColorEnabled` line-for-line (the proposal's decision to duplicate rather than refactor is binding — `simplify` says three similar lines is fine, and a third gate may diverge later): + +1. Read `process.env.NO_COLOR`. If it is a string of length > 0 → return `false`. +2. Read `process.env.FORCE_COLOR`. If non-empty: + - lowercase it + - if `"0"` or `"false"` → return `false` + - else → return `true` (overrides `isTTY`) +3. Fallback: return `stream.isTTY === true`. + +This guarantees the Success Criterion cases: + +| Env / stream | Result | +|--------------|--------| +| `isTTY=true`, no env | `true` | +| `isTTY=false`, no env | `false` | +| `isTTY=true`, `NO_COLOR=1` | `false` | +| `isTTY=true`, `FORCE_COLOR=0` | `false` | +| `isTTY=false`, `FORCE_COLOR=1` | `true` (override) | +| `isTTY=true`, `NO_COLOR=1` + `FORCE_COLOR=1` | `false` (NO_COLOR wins, no-color.org) | + +### `withLoading` — TTY-false path (no-op) + +1. Check `isSpinnerEnabled(stream)`. Returns `false`. +2. Return `fn()` directly. No `setInterval`, no `stream.write`, no `try/finally` overhead. +3. Caller receives the resolved `T` (or the rejection) untouched. + +Observable behavior: stream gets zero writes from the helper. Byte-identical to calling `fn()` directly. This is the path taken by `CI=true`, `2>/dev/null`, `NO_COLOR=1`, and any non-interactive harness. + +### `withLoading` — TTY-true path (animated) + +1. Check `isSpinnerEnabled(stream)`. Returns `true`. +2. Compute `frame = SPINNER_FRAMES[0]` and `width = 1` (every Braille frame is one column wide — invariant baked into the frame array). +3. Initialize index `i = 0`. +4. Start `setInterval` at `options?.interval ?? 80` ms. Each tick: + - `stream.write("\r" + SPINNER_FRAMES[i % SPINNER_FRAMES.length])` + - `i++` + - No newline. The `\r` returns cursor to column 0; next frame overwrites the glyph in place. +5. Write the initial frame **immediately** (before awaiting `fn`) so the user sees the spinner appear without waiting up to one interval. Without this, fast-resolving `fn` would never render and the spinner would feel intermittent. +6. `try { return await fn(); } finally { cleanup() }` where `cleanup()`: + - `clearInterval(handle)` + - `stream.write("\r" + " ".repeat(width) + "\r")` — erase the glyph, return cursor to column 0 +7. Caller receives the resolved `T` after cleanup completes. + +Byte sequence on a successful single-tick path: + +``` +write: "\r⠋" // initial render (step 5) +[interval may fire 0..N times] +write: "\r⠙" // tick 1 +write: "\r⠹" // tick 2 +[fn resolves] +write: "\r \r" // cleanup (step 6) +``` + +After cleanup the cursor is at column 0 of the spinner line with that column blanked — the caller's next stdout write (the verse text + `\n`) lands cleanly on the row the spinner occupied. Because the verse text goes to **stdout** and the spinner is on **stderr**, in interactive terminals both streams render to the same TTY device and the cleanup `\r` + space + `\r` correctly erases the visible glyph. + +### `withLoading` — rejection path + +`finally` runs whether the `try` block resolves or rejects. The cleanup write happens before the rejection escapes the wrapper. Sequence when `fn` rejects: + +1. Same setup as the TTY-true path above (or skipped entirely on TTY-false). +2. `await fn()` throws (rejects). +3. `finally` block executes: `clearInterval(handle)`, then `stream.write("\r \r")`. +4. The rejection propagates out of `withLoading` to the caller. + +Important: `withLoading` does NOT catch the rejection or convert it to a `Result`. R1 talks about **domain** functions; this is presentation. The convention in the codebase is that `getPassage` already returns `Promise>` — a rejection from `withLoading`'s `fn` would only happen for a programmer error (e.g. a thrown TypeError inside the thunk), which should surface as an unhandled rejection at the process boundary, not be silently absorbed. R1's spirit (no exception masking) is honored. + +### Concurrency — single spinner at a time + +`withLoading` is not re-entrant. Two concurrent `withLoading` calls writing to the same stream would interleave `\r`-anchored writes and produce garbage. The codebase has exactly two call sites (`run.ts:39`, `vod.ts:40`), each of which is the sole `await` in its `run`/`runVod` function. There is no path through `run.ts` or `vod.ts` that nests `withLoading`. This is a binding constraint on the helper, not a guarded invariant — the helper does NOT track an "in flight" flag. Documentation in `loading.ts` calls this out; the test for nested behavior is intentionally absent (no need to lock down something we've contracted out of). + +Future use sites (e.g. a TUI loading state, or a CLI subcommand that issues two parallel fetches) must NOT call `withLoading` concurrently against the same stream. If a future feature needs parallel work indicators, that is a separate design — not v1. + +## House Rules Compliance + +| Rule | Verdict | Justification | +|------|---------|---------------| +| R1 — domain never throws | N/A (presentation) | `withLoading` itself never throws; `fn`'s rejection propagates unchanged via `finally` — no exception masking. Spirit preserved. | +| R2 — no `class` outside `src/tui/` | Respected | `withLoading` and `isSpinnerEnabled` are plain functions. `SPINNER_FRAMES` is a const tuple. | +| R3 — ports have no callbacks | N/A | `withLoading` is a CLI presentation utility, not a port. The `fn` parameter is a thunk for deferred execution, not an event handler on a hexagonal-boundary interface. R3 is binding for `BibleRepository`-style interfaces only. | +| R4 — Zod stays in `src/api/` | N/A | No validation involved. | +| R5 — errors as discriminated unions with `kind` | Respected | No new error types introduced. Caller's `Result` (which already has `kind` on its error branch) flows through unchanged. | +| R6 — branded IDs via single factory | N/A | No IDs introduced. | +| R7 — no conditional/mapped/template-literal types in domain/application | N/A (and respected anyway) | `loading.ts` is presentation. The signature uses a single generic `T` and an `as const` literal tuple — no mapped/conditional/template-literal types. | +| R8 — TUI business state in `useReducer` | N/A | CLI has no React state. | +| R9 — no `useEffect` for business logic | N/A | CLI is imperative; `setInterval` is the correct primitive for time-based UI in a non-React surface. | +| R10 — action names past-tense | N/A | No reducer actions. | +| R11 — no decorators | Respected | `withLoading` is a plain higher-order function. R11 prohibits decorators, not HOFs — explicit higher-order functions are the recommended substitute. | +| R12 — async data fns return `Promise>` | Respected | `withLoading` is transparent to `T`. When callers pass `() => getPassage(repo, ref)`, `T = Result` and the helper returns that exact shape. The wrapper does NOT re-shape the Result. | + +cognitive-doc-design: this design leads with the architecture diagram (Module Layout), then the type signatures, then control flow as numbered steps — show, don't tell. + +simplify: `isSpinnerEnabled` deliberately duplicates `isColorEnabled` rather than extracting a shared `isTTYEnabled`. Three similar lines is fine; the responsibilities are conceptually distinct (color vs animation) and a future divergence is a 5-line edit, not an API refactor. + +## Test Design + +### `src/cli/loading.test.ts` (NEW, ~80 lines) + +Follows the structure of `src/cli/ansi.test.ts` exactly: `describe` blocks, `beforeEach`/`afterEach` for env-var save/restore, fake streams cast from POJOs. + +Imports: + +```ts +import { describe, it, expect, beforeEach, afterEach } from "bun:test"; +import { withLoading, isSpinnerEnabled, SPINNER_FRAMES } from "./loading"; +``` + +Fake-stream factory (one helper, defined once per file): + +```ts +type WriteCall = string; +function fakeStream(isTTY: boolean): { + stream: NodeJS.WriteStream; + writes: WriteCall[]; +} { + const writes: WriteCall[] = []; + const stream = { + isTTY, + write: ((chunk: string | Uint8Array) => { + writes.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); + return true; + }) as NodeJS.WriteStream["write"], + } as NodeJS.WriteStream; + return { stream, writes }; +} +``` + +Test groups: + +**Group 1 — `isSpinnerEnabled` truth table** (mirrors `ansi.test.ts` T1-T9): + +| ID | Stream | NO_COLOR | FORCE_COLOR | Expect | +|----|--------|----------|-------------|--------| +| S1 | TTY | unset | unset | `true` | +| S2 | pipe | unset | unset | `false` | +| S3 | TTY | `"1"` | unset | `false` | +| S4 | TTY | `""` | unset | `true` (empty = no opinion) | +| S5 | pipe | unset | `"1"` | `true` (override) | +| S6 | TTY | unset | `"0"` | `false` | +| S7 | TTY | unset | `"false"` | `false` | +| S8 | pipe | `"1"` | `"1"` | `false` (NO_COLOR wins) | + +Use the same `beforeEach`/`afterEach` env-var save/restore block as `ansi.test.ts:36-57`. + +**Group 2 — `withLoading` TTY-false path (no-op)**: + +- `T-NOOP-1`: `isTTY=false`, fn returns `42` immediately → no writes recorded, fn called exactly once, result is `42`. Use `interval: 1_000_000` defensively (interval should never start, but in case of a bug we want zero risk of ticks). +- `T-NOOP-2`: `isTTY=false`, fn returns `{ ok: true, value: "x" } as const` (Result shape) → result passes through with referential equality. + +**Group 3 — `withLoading` TTY-true path (renders + cleans up)**: + +- `T-TTY-1`: `isTTY=true`, fn resolves with `7`, `interval: 1_000_000` (no tick fires during the test). Assert: + - `writes.length >= 2` (initial render + cleanup) + - `writes[0] === "\r" + SPINNER_FRAMES[0]` (initial render) + - `writes[writes.length - 1] === "\r \r"` (cleanup sequence — `\r` + 1 space + `\r`) + - returned value === `7` +- `T-TTY-2`: same setup but `interval: 1`, fn awaits one microtask before resolving. Assert cleanup is still the last write. (This is the "real-tick survival" test — kept simple to avoid timing flakiness.) + +**Group 4 — `withLoading` rejection path**: + +- `T-REJECT-1`: `isTTY=true`, fn rejects with `new Error("nope")`. Assert: + - the test awaits `withLoading` inside `expect(...).rejects.toThrow("nope")` (Bun supports this) + - after the rejection settles, inspect `writes`: last entry is `"\r \r"` (cleanup ran before rejection propagated) + +**Group 5 — frame array shape**: + +- `T-FRAMES-1`: `SPINNER_FRAMES.length === 10` +- `T-FRAMES-2`: every frame is a single grapheme (string length 1 in JS for these Braille code points — they are all in the BMP, single code units) + +Total: ~14-16 `it` blocks, well under 100 lines. + +### Smoke coverage — extend `tests/vod-smoke.test.ts` + +Decision: **extend `tests/vod-smoke.test.ts`** rather than create `tests/loading-smoke.test.ts`. Rationale: the existing file already monkey-patches `process.stderr.write` and runs `runVod` end-to-end. A new file would duplicate the harness for a single assertion. One additional `describe` block keeps the smoke surface flat. + +Add this block to `tests/vod-smoke.test.ts`: + +```ts +describe("smoke — verbum vod loading state is invisible to redirected pipelines", () => { + it("captured stderr contains no spinner frames when isTTY is false", async () => { + // Fixed date so the picker is deterministic; stubRepo resolves synchronously. + const fixed = new Date(2025, 5, 15); + + // Force the no-spinner path: NO_COLOR=1 disables isSpinnerEnabled regardless of isTTY. + const savedNoColor = process.env.NO_COLOR; + process.env.NO_COLOR = "1"; + + let stderrCapture = ""; + const origStderr = process.stderr.write.bind(process.stderr); + process.stderr.write = ((chunk: string | Uint8Array) => { + stderrCapture += typeof chunk === "string" ? chunk : Buffer.from(chunk).toString(); + return true; + }) as typeof process.stderr.write; + + try { + await runVod(fixed, stubRepo); + } finally { + process.stderr.write = origStderr; + if (savedNoColor === undefined) delete process.env.NO_COLOR; + else process.env.NO_COLOR = savedNoColor; + } + + // Spinner frames and the cleanup sequence must NOT appear in captured stderr. + for (const frame of ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]) { + expect(stderrCapture).not.toContain(frame); + } + expect(stderrCapture).not.toContain("\r"); + }); +}); +``` + +Why `NO_COLOR=1` instead of `isTTY=false`: `process.stderr.isTTY` is a runtime property of the global process object that we cannot safely mutate in-test without risking pollution. `NO_COLOR=1` flips `isSpinnerEnabled` to `false` via the env-var path with no side effects on the global stream object. The env var is saved/restored in `finally`. + +There is no smoke test that proves the spinner **does** render — the smoke harness cannot simulate a TTY without a real PTY, which is out of scope for `bun test`. The unit tests in Group 3 cover that path via fake streams. This is the same trade-off `ansi.test.ts` makes for `isColorEnabled`. + +## File-by-file Change Plan + +| File | Action | Specifics | +|------|--------|-----------| +| `src/cli/loading.ts` | NEW (~50 lines) | Exports `SPINNER_FRAMES` (`as const`), `WithLoadingOptions`, `SpinnerFrame`, `isSpinnerEnabled`, `withLoading`. Internal: cleanup helper inlined into `finally`. | +| `src/cli/loading.test.ts` | NEW (~80 lines) | Groups 1-5 above. Reuses env-var save/restore pattern from `ansi.test.ts`. | +| `src/cli/run.ts` | MODIFY | Add `import { withLoading } from "@/cli/loading";` after the existing `@/cli/render` import. Change line 39 from `const passageResult = await getPassage(repo, refResult.value);` to `const passageResult = await withLoading(process.stderr, () => getPassage(repo, refResult.value));`. | +| `src/cli/vod.ts` | MODIFY | Add `import { withLoading } from "@/cli/loading";` after the existing `@/cli/render` import. Change line 40 from `const passageResult = await getPassage(repo, ref);` to `const passageResult = await withLoading(process.stderr, () => getPassage(repo, ref));`. | +| `tests/vod-smoke.test.ts` | MODIFY | Append one `describe` block (the loading-state smoke assertion above). No change to existing test bodies. | + +Net diff: ~150 lines added, 2 lines modified, 2 imports added. One PR. + +## Risks and Mitigations + +| Risk | Mitigation | +|------|------------| +| `setInterval` outlives the test that started it → flaky stderr captures, Bun "timer not cleared" warning | Cleanup in `finally` runs before `await` returns to the caller; tests `await` the wrapper so cleanup is synchronous w.r.t. assertions. Unit tests inject `interval: 1_000_000` to make the interval effectively dormant during the test body. | +| Spinner frame leaks into stdout pipelines (`verbum john 3:16 \| jq`) | Spinner target is **stderr**, set in the call site (`withLoading(process.stderr, ...)`); stdout is never written by the helper. Smoke test asserts stderr is empty when `NO_COLOR=1`. | +| `NO_COLOR=1` ignored by spinner (Success Criterion regression) | `isSpinnerEnabled` is its own function and is the gate — it implements the same `NO_COLOR` priority chain as `isColorEnabled`. Group 1 truth-table tests lock this down. | +| Line-wrap orphan if spinner glyph + future text exceeds terminal width | Spinner is frame-only (no text — proposal decision). 1-column glyph is unambiguously safe at any width ≥ 1. Cleanup writes exactly `width` spaces. | +| Nested / concurrent `withLoading` calls produce interleaved garbage | `withLoading` is documented as single-instance-per-stream. The two production call sites are mutually exclusive (different subcommands) and each is the sole `await` in its function. No nested path exists in v1. | +| `process.stderr.isTTY` differs between dev terminal and CI | The env-var override chain (`FORCE_COLOR=0` / `NO_COLOR=1`) provides explicit opt-out. CI harnesses with `NO_COLOR=1` set globally get no spinner. | +| `\r` cleanup ineffective on terminals that wrap the spinner line | Unreachable: frame width is 1, terminal min width ≥ 40 cols. The wrap path cannot trigger. | +| Spinner appears in `bun test` output when running tests interactively | Unit tests pass fake streams that never write to a real terminal; smoke test sets `NO_COLOR=1`. No production code in `bun test` triggers the spinner against `process.stderr`. | + +## Out of Scope + +Verbatim from the proposal — restated here so the apply phase has a single closed envelope: + +- Loading state for any future TUI screens (different mechanism via OpenTUI/React) +- Progress percentage or determinate progress bars (indeterminate spinner only) +- Custom themes or user-configurable spinner frames +- Multi-line layouts, gradient frames, fancy rendering +- A `--no-spinner` flag (`NO_COLOR` and `isTTY` already provide opt-out) +- Refactoring `isColorEnabled` to share implementation with `isSpinnerEnabled` +- Spinner for any non-`getPassage` operation +- Replacing the hand-rolled spinner with `ora` or another library +- Bun fake-timer infrastructure (injectable `interval` is the v1 strategy) +- Nested / concurrent `withLoading` against the same stream + +## Open Design Questions + +None. All four exploration questions are decided in the proposal; the design phase only resolves implementation-level concerns (interval handle type, initial-render-before-tick, smoke harness choice) — each is locked down above. diff --git a/openspec/changes/cli-loading-state/explore.md b/openspec/changes/cli-loading-state/explore.md new file mode 100644 index 0000000..71f4e86 --- /dev/null +++ b/openspec/changes/cli-loading-state/explore.md @@ -0,0 +1,159 @@ +# Exploration — cli-loading-state + +## TL;DR +- **What**: A shared `src/cli/loading.ts` helper that renders an animated spinner to **stderr** while a blocking async call runs, then clears the line before the caller writes output to stdout. +- **Where**: New file `src/cli/loading.ts`; consumed by `src/cli/run.ts` (line 39 — `getPassage` await) and `src/cli/vod.ts` (line 40 — `getPassage` await). No other files need changing. +- **API**: Async-wrapper form — `await withLoading(process.stderr, () => getPassage(repo, ref))` — is the cleanest fit. Reasoning in Design Space below. +- **Hand-rolled vs library**: Hand-rolled. verbum has zero utility deps (`zod`, `@opentui/*`, `react` are all product deps). A 30-line spinner is cheaper than `ora`'s 7-dep chain and a `package.json` entry that needs auditing forever. +- **Riskiest unknown**: Spinner interval leaking across test runs when stdout/stderr are monkey-patched. `clearInterval` in the finally block is mandatory; tests must be able to inject a fake clock or a no-op writer. + +## Current State + +### Files read + +| File | What it does today | Gap | +|------|--------------------|-----| +| `src/cli/run.ts:39` | `await getPassage(repo, ref)` — blocks 1-3s, no feedback | blank terminal | +| `src/cli/vod.ts:40` | same `await getPassage(repo, ref)` — blocks 1-3s, no feedback | blank terminal | +| `src/cli/render.ts` | pure formatters: `renderPassage`, `renderParseError`, `renderRepoError`. Uses `isColorEnabled(stream)` gating. No IO side effects. | no gap | +| `src/cli/ansi.ts` | `isColorEnabled(stream)` reads `process.stdout.isTTY` / env-vars; `accent`, `dim`, `error`, `muted` pure wrappers. Already has the full TTY/env detection pattern. | no gap — reuse directly | +| `src/cli/ansi.test.ts` | Tests `isColorEnabled` via `{ isTTY: true/false }` cast + env-var mutation+restore with `beforeEach/afterEach`. | Pattern to clone for loading tests | +| `tests/vod-smoke.test.ts` | Monkey-patches `process.stdout.write` and `process.stderr.write` with try/finally restore. | Pattern to clone for loading tests | +| `package.json` | deps: `@opentui/core`, `@opentui/react`, `react`, `zod`. devDeps: `@types/figlet`, `@types/react`, `figlet`. **Zero utility deps.** | Cost of adding `ora` is non-zero | + +### Network blocking point + +Both commands call a single function: `getPassage(repo, ref)` from `src/application/get-passage.ts`, which delegates to `BibleRepository.getChapter` — the actual `fetch()` in `src/api/hello-ao-bible-repository.ts:32`. This is the **sole blocking point** in both commands. The spinner wraps exactly this `await`. + +## Design Space + +### 1. Where the helper lives + +`src/cli/loading.ts` is correct. Rationale: + +- It is a **presentation** concern, not domain or application. It writes to stderr, reads TTY state, manages timers — all IO-side-effects that belong in the CLI adapter layer. +- The existing CLI layer already has `ansi.ts` for ANSI escape helpers. `loading.ts` is a peer, not a sub-module. +- There is no existing `src/cli/presentation/` subdirectory and no pattern of subdirectories within `src/cli/`. Adding one for a single file would be premature structure. +- `src/presentation/cli/` would suggest a separate presentation layer outside the current hexagonal layout; the project does not have this concept. + +**Decision**: `src/cli/loading.ts`. + +### 2. Consumption API + +Three options: + +| Option | Signature | House rule verdict | +|--------|-----------|-------------------| +| Imperative start/stop | `const stop = startLoading(stderr); ... stop()` | Requires caller to manually call `stop()` in every return path — error-prone with multiple early returns in `run.ts` and `vod.ts`. | +| Async wrapper | `await withLoading(stderr, () => asyncFn())` | Single try/finally inside the helper guarantees cleanup. No callback persisted → no port concern (R3 says ports have no callbacks; this is NOT a port, it's a presentation utility). | +| Declarative/lifecycle | React-style hook | Inappropriate for CLI layer. R9 applies to TUI layer only; CLI is imperative. No framework here. | + +**Decision: async wrapper** — `withLoading(stream: NodeJS.WriteStream, fn: () => Promise): Promise`. + +R3 note: R3 prohibits callbacks on **ports** (hexagonal interfaces between layers). `withLoading` is a CLI presentation utility — it never crosses the hexagonal boundary, it is not a port, and R3 explicitly covers `BibleRepository`-style interfaces. The callback here is a thunk for async execution, not an event handler on an interface. This is safe. + +R9 note: R9 targets `useEffect` in the TUI React layer. CLI code is pure imperative TS with no React lifecycle. Not applicable. + +### 3. Hand-rolled vs library + +**Verdict: hand-rolled.** + +Current dependency count: 4 prod deps (`@opentui/core`, `@opentui/react`, `react`, `zod`). verbum has a strict no-utility-dep posture (confirmed by absence of lodash, chalk, ora, etc.). + +`ora` costs: 7 transitive deps, ESM-only (requires `"type": "module"` alignment check with Bun compile), ~15KB unpacked, version drift risk, audit surface. + +Hand-rolled spinner cost: ~30 lines. Frame array (`["⠋","⠙","⠹","⠸","⠼","⠴","⠦","⠧","⠇","⠏"]`), `setInterval` at 80ms, `\r` + frame + text on each tick, `\r` + spaces + `\r` on cleanup. Uses `isColorEnabled(stream)` already in `ansi.ts` to gate ANSI use. No new dependencies. + +**The break-even for a library is when the hand-rolled version would exceed ~100 lines or require significant ongoing maintenance.** A spinner does not meet that bar. + +### 4. TTY detection contract + +`isColorEnabled` in `src/cli/ansi.ts` already implements the canonical detection: +1. `NO_COLOR` (non-empty) → disable +2. `FORCE_COLOR` (`"0"` or `"false"`) → disable; anything else non-empty → enable +3. Fallback: `stream.isTTY === true` + +The loading helper needs a **separate but analogous** check: `isSpinnerEnabled(stream: NodeJS.WriteStream): boolean`. It should follow identical priority order. Reusing `isColorEnabled` directly would conflate "can render color" with "should animate" — these are independent. However, the implementation is identical in v1 (both defer to `isTTY`). + +**When `isTTY` is false** (the helper is a no-op — no writes to stderr): +- `verbum john 3:16 > output.txt` — stdout redirected, stderr may still be TTY but stdout is not; spinner target is stderr so this case is actually fine. **Spinner on stderr is still valid here.** +- `verbum john 3:16 | wc` — stdout piped; stderr still a TTY. Spinner is valid. +- `verbum john 3:16 2>/dev/null` — stderr explicitly discarded; `process.stderr.isTTY` will be false. No spinner, no waste. +- CI environments — `CI=true` disables TTY allocation; `process.stderr.isTTY` is `undefined`/`false`. Correct: no spinner. +- Non-interactive shell scripts — TTY not allocated; `isTTY` is false. No spinner. + +**Conclusion**: checking `process.stderr.isTTY` is the correct and sufficient gate. The spinner target is stderr, so stdout redirection does not interfere. + +### 5. Render target + +**stderr is correct.** Rationale: + +- `run.ts` writes final verse text to **stdout** (`process.stdout.write(renderPassage(...) + "\n")`). +- `run.ts` writes errors to **stderr** (`process.stderr.write(renderParseError(...) + "\n")`). +- The spinner must not appear in stdout — it would corrupt `verbum john 3:16 | jq` or `> file.txt` pipelines. +- Writing spinner frames to stderr keeps stdout clean for downstream consumers. + +Are there cases where stderr is also piped? Yes (`2>file` or `|&`). When stderr is redirected, `process.stderr.isTTY` is false → spinner is already suppressed. No special handling needed. + +### 6. Cleanup contract + +**Mechanism**: `\r` (carriage return) + spaces (to width of last frame string) + `\r`. This overwrites the spinner line without an ANSI clear-line escape, maximizing terminal compatibility. + +ANSI clear-line (`\x1b[2K`) is more elegant but requires ANSI support. Using `\r` + spaces is universally safe and avoids a conditional. + +**Line wrap risk**: if the spinner text + frame exceeds terminal width, the line wraps and `\r` only returns to the start of the current line, leaving an orphaned first line. Mitigation: keep spinner text short (e.g. `" loading…"` — 10 chars) plus the frame (1 char) = 11 chars. Well within any realistic terminal width (minimum 40 cols). No truncation logic needed. + +**Cleanup sequence** (must happen before any stdout/stderr writes from the caller): +``` +clearInterval(handle) +stderr.write("\r" + " ".repeat(frameLen) + "\r") +``` +The `withLoading` wrapper runs cleanup in `finally`, so it executes before the resolved/rejected value is returned to the caller. + +### 7. Test approach + +**Pattern already established** in `tests/vod-smoke.test.ts` and `src/cli/ansi.test.ts`: + +For unit tests of `loading.ts`: +- Pass a fake stream `{ isTTY: false, write: () => true }` → assert helper is a no-op (no writes, just returns the fn result). +- Pass a fake stream `{ isTTY: true, write: captureFn }` → assert that write was called at least once (spinner rendered), and that cleanup write (`\r...`) was the last call. +- Use `beforeEach/afterEach` for `NO_COLOR`/`FORCE_COLOR` env-var mutation/restore (identical pattern to `ansi.test.ts`). + +**Timer leak concern**: `setInterval` in the helper will tick during tests. Bun's test runner does not automatically fake timers. Options: +- Option A: pass an `interval` parameter (default 80ms) and set it to a large value in tests (e.g., 1_000_000ms) so ticks never fire during the test. +- Option B: use `Bun.spyOn` / manual mock for `setInterval` if Bun supports it. +- Option C: the wrapper resolves so fast in tests (no real network) that interval never ticks; cleanup still fires in `finally`. This is the simplest approach and matches how `vod-smoke.test.ts` works — the fn is sync/immediate in tests. + +**Recommended**: Option C for smoke tests (fn resolves immediately, interval never fires). Option A for dedicated unit tests of spinner behavior (inject large interval to control tick timing). + +For integration: existing smoke test pattern (`process.stderr.write` monkey-patch in try/finally) extends cleanly. + +**Bun compatibility note**: Bun's test runner runs tests concurrently within a file. Monkey-patching `process.stderr.write` globally is not safe across concurrent tests. The existing smoke tests serialize via `describe` blocks (Bun runs `it` blocks within a `describe` sequentially). Continue this pattern. + +### 8. Riskiest unknown + +**Interval leak in tests.** If `withLoading` starts a `setInterval` and the test completes before cleanup fires, the interval continues ticking into the next test. In practice, `withLoading` always calls `clearInterval` in `finally`, so as long as `await withLoading(...)` is awaited (not fire-and-forget), cleanup is guaranteed before the test assertion. The risk is a test that does NOT await the call — this cannot happen given how the helper is used in `run.ts` and `vod.ts`. + +Secondary risk: **terminal width assumption for cleanup**. Using `" ".repeat(N)` requires knowing `N`. We know the spinner text at construction time, so N = `frame.length + text.length`. This is computed inside the helper and is safe. + +### 9. Existing patterns to reuse + +- `isColorEnabled(stream)` from `src/cli/ansi.ts` — reuse the same logic (or extract a shared `isTTYEnabled(stream)` if we want semantic separation). The implementation is identical. +- Monkey-patch pattern for `process.stderr.write` from `tests/vod-smoke.test.ts` — reuse directly for loading tests. +- `ACCENT_OPEN` / `DIM_OPEN` from `src/cli/ansi.ts` if the spinner frame should be accent-colored (the DIM constant exists already for muted text). +- No existing spinner, loader, or progress helper exists in the codebase. + +## Recommendation (single path forward) + +Create `src/cli/loading.ts` exporting a single function `withLoading(stream: NodeJS.WriteStream, fn: () => Promise): Promise`. Inside: check `stream.isTTY` (same logic as `isColorEnabled`), return `fn()` directly if false (no-op), otherwise start a `setInterval` at 80ms cycling through 10 Braille spinner frames written to `stream` via `\r`, then in `finally` clear the interval and overwrite the line with spaces. Modify `src/cli/run.ts:39` and `src/cli/vod.ts:40` to wrap the `getPassage` call with `withLoading(process.stderr, () => getPassage(repo, ref))`. The helper respects R1 (never throws — `fn`'s Result passes through unchanged), R5 (no new error types), R12 (returns `Promise>`), and R3 (not a port — it is a CLI presentation utility below the hexagonal boundary). Test surface: unit tests mirroring `ansi.test.ts` pattern + existing smoke tests extend cleanly via the monkey-patch pattern. + +## Riskiest Unknown + +Interval timer interaction with Bun's test runner. The `setInterval` must be cleared in `finally` before the resolved value propagates. Tests that pass an immediately-resolving function (the smoke test pattern) are safe because the interval never ticks before `finally` runs. Tests that deliberately test spinner output need to either use a large interval value or spy on `setInterval`/`clearInterval`. If the apply phase uses Option C (rely on fast test functions), no fake timer infrastructure is needed — this is the path of least resistance. + +## Open Questions for Proposal Phase + +1. **Spinner text copy**: what does the spinner say? `" loading…"`, `" fetching verse…"`, silent (frame only)? The proposal should lock down the exact string. +2. **Accent color on spinner**: should the spinner frame use the project accent color (`#5BA0F2`) via `ansi.ts`'s `ACCENT_OPEN`, or remain plain white? The accent is already used for emphasis; using it for the spinner frame is consistent but optional. +3. **`isSpinnerEnabled` vs reusing `isColorEnabled`**: proposal should decide whether to introduce a named `isSpinnerEnabled` function (clearer semantics) or inline the `stream.isTTY` check (simpler). The implementations are identical in v1. +4. **Test strategy for spinner output**: Option A (inject large interval) or Option C (rely on fast fn). Proposal should pick one and document it as the project convention for future animated helpers. diff --git a/openspec/changes/cli-loading-state/proposal.md b/openspec/changes/cli-loading-state/proposal.md new file mode 100644 index 0000000..d4728fd --- /dev/null +++ b/openspec/changes/cli-loading-state/proposal.md @@ -0,0 +1,131 @@ +# Proposal — cli-loading-state + +## TL;DR + +- Ship `withLoading(stream, fn)` in `src/cli/loading.ts` — a hand-rolled, stderr-only Braille spinner that wraps the single `getPassage` await in `run.ts` and `vod.ts`. +- First reviewable cut: one PR, ~5 files touched, ~30 lines of logic + unit tests + a smoke assertion that piped stderr stays empty. +- Success: `verbum john 3:16` shows an animated spinner on stderr during fetch; `verbum john 3:16 | jq` and `verbum john 3:16 2>/dev/null` produce byte-identical output to today; `bun test` stays green with no timer leaks across files. +- Riskiest unknown: `setInterval` outliving the test that started it. Mitigated by `finally`-clear + an injectable `interval` parameter for the one test that exercises a real tick. + +## Intent + +Today, after the user types `verbum john 3:16` and presses enter, the terminal sits blank for 1–3 seconds while `getPassage` fetches over the network, then the verse text appears all at once. There is no signal that work is happening — indistinguishable from a hung process on a slow connection. After this change, the same command writes a spinner frame (Braille `⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏`) to **stderr** every 80 ms while the fetch is in flight, then erases the spinner line cleanly so the verse text lands on stdout exactly as it does today — pristine, pipeable, redirect-safe. In non-TTY contexts (CI, `2>/dev/null`, scripts) the helper is a strict no-op: zero stderr writes, zero behavior change. + +## First Reviewable Cut + +- [ ] Create `src/cli/loading.ts` exporting `withLoading(stream: NodeJS.WriteStream, fn: () => Promise, options?: { interval?: number }): Promise` +- [ ] Create `src/cli/loading.test.ts` with unit tests covering: no-op when `isTTY=false`, write-then-cleanup when `isTTY=true`, cleanup runs when `fn` rejects, `NO_COLOR`/`FORCE_COLOR` env-var override behavior +- [ ] Modify `src/cli/run.ts:39` — wrap `getPassage(repo, ref)` with `withLoading(process.stderr, () => getPassage(repo, ref))` +- [ ] Modify `src/cli/vod.ts:40` — wrap the same call identically +- [ ] Extend `tests/vod-smoke.test.ts` (or add a sibling `tests/loading-smoke.test.ts`) to assert: captured stderr is empty when the smoke test's fake stream reports `isTTY=false`, proving redirected pipelines stay byte-clean + +No other files change. No new dependencies. No package.json edits. + +## Success Criterion + +A reviewer can run each line below and observe the stated outcome: + +- `verbum john 3:16` in an interactive terminal → animated spinner on stderr during fetch, single clean line of verse text on stdout after fetch resolves +- `verbum john 3:16 2>/dev/null` → stdout shows the verse, no spinner artifacts anywhere (stderr discarded, but no escape codes leaked to stdout) +- `verbum john 3:16 > out.txt` → `out.txt` contains only the verse text, byte-identical to today's output +- `verbum john 3:16 | wc -c` → exit 0, byte count matches today's output (stdout untouched) +- `CI=true verbum john 3:16` in a non-TTY harness → no spinner frames written to stderr +- `NO_COLOR=1 verbum john 3:16` → no spinner frames (spinner gate honors `NO_COLOR` exactly like `isColorEnabled` does) +- `bun test` passes including new `src/cli/loading.test.ts` and the smoke addition; no "Bun detected a timer that was not cleared" warnings; total test time does not regress +- `verbum vod` exhibits identical spinner behavior (the helper is the only change to that path) + +## Riskiest Unknown + +`setInterval` outliving the test that started it. If `withLoading` starts a 80 ms interval and the test under `bun test` finishes asserting before `finally` clears it, the interval ticks into the next test in the file, randomly polluting stderr captures and possibly tripping Bun's open-handle warning at process exit. + +Mitigation, layered: + +1. **Always-on**: `withLoading` calls `clearInterval` in `finally`, which runs before the `await` returns to the caller. As long as the test `await`s the wrapper (which it must, because the helper returns `Promise` and callers consume `T`), cleanup is guaranteed before the next assertion. +2. **Test-side**: the `interval` option (default `80`) lets the one unit test that wants to exercise a real tick pass `interval: 1` (force immediate tick) or `interval: 1_000_000` (guarantee no tick during the test body). This keeps the production default invisible while making timing-sensitive tests deterministic. +3. **Smoke-side**: smoke tests pass a fast-resolving `fn` (no real network) — the interval never fires, `finally` clears it, no observable timer artifact. + +If despite all this `bun test` reports leaked handles after apply, the fallback is `Bun.spyOn(globalThis, "setInterval")` — but this is not the v1 plan. + +## Decisions Locked + +### Spinner text copy + +**Decision**: `""` — frame only, no text. + +**Rationale**: The spinner's job is to signal "work is happening." A single Braille glyph already does that; adding `" loading…"` or `" fetching verse…"` doubles the visual footprint without adding information, and pulls vocabulary into the CLI that we would then have to localize and audit when the API surface changes (e.g. when a future cache hit makes "fetching" a lie). Frame-only also dodges the line-wrap edge case entirely — 1 character is unambiguously safe at any terminal width. The `cognitive-doc-design` rule "lead with the decision, not the context" applies to UI too: silence is the minimum signal. + +### Accent color on spinner frame + +**Decision**: No. Plain default-foreground frame. + +**Rationale**: The `#5BA0F2` accent is reserved as a **structural** signal in `renderPassage` (reference labels, emphasis points). Using it on the spinner — a transient, non-structural element — dilutes that role. Plain monochrome also avoids a second branch in the helper (`isColorEnabled` vs `isSpinnerEnabled`) and keeps the spinner readable on terminals where `#5BA0F2` is close to the background. If a future change introduces a deliberate "in-flight" color token, the spinner can adopt it then; today there is no such token, and inventing one for a transient glyph is premature. + +### Spinner gate function (`isSpinnerEnabled` vs inline `stream.isTTY`) + +**Decision**: Introduce `isSpinnerEnabled(stream: NodeJS.WriteStream): boolean`, exported from `src/cli/loading.ts` (NOT `ansi.ts`). + +**Rationale**: This is a deliberate deviation from the exploration's default recommendation (inline `stream.isTTY`). The reason: `simplify` says reuse before abstraction, but it also says question every new abstraction. Inline `stream.isTTY` skips the `NO_COLOR` / `FORCE_COLOR` priority chain that `isColorEnabled` already honors — and the Success Criterion explicitly tests `NO_COLOR=1 verbum john 3:16`. A bare `stream.isTTY` check would fail that test. The choices are: + +| Option | Result | +|--------|--------| +| Inline `stream.isTTY` only | Spinner ignores `NO_COLOR` → fails Success Criterion | +| Call `isColorEnabled` from `loading.ts` | Conflates "should color" with "should animate" — defensible today, awkward when these diverge (e.g. if `NO_COLOR=1` should keep the spinner but disable color) | +| `isSpinnerEnabled` exported from `loading.ts`, implementation identical to `isColorEnabled` | Clear semantic boundary, ~5 lines duplicated but each function has a single responsibility, future divergence is a 5-line edit not an API change | + +Three similar lines is fine (`simplify`), but the responsibilities differ. House-rule note: no rule is violated — this is a CLI presentation helper, not a port (R3 N/A), not a domain function (R1/R4/R12 N/A). + +### Test strategy + +**Decision**: Both — Option A (injectable `interval` parameter) for `src/cli/loading.test.ts`, Option C (rely on fast-resolving `fn`) for the smoke addition in `tests/`. + +**Rationale**: Option C alone cannot test the spinner actually rendering a frame (the `fn` resolves before the first tick fires), and Option A alone overspecifies the smoke surface. Pairing them gives: + +- Unit (`loading.test.ts`): inject `interval: 1` to force a tick, capture writes to the fake stream, assert the last write is the cleanup sequence (`\r` + spaces + `\r`). Inject `interval: 1_000_000` for tests that only care about gate behavior and want zero ticks. Deterministic, no timer leak. +- Smoke (`tests/`): pass a real-shaped `fn` that resolves synchronously, prove the wrapper is transparent to the caller's contract (Result flows through unchanged, stdout stays clean). The interval starts and is cleared by `finally` before the assertion runs — exactly like the existing `vod-smoke.test.ts` pattern. + +This establishes the project convention: animated CLI helpers expose a timing parameter for unit testability, but smoke/integration tests rely on fast functions and `finally`-cleanup, not fake clocks. + +## Affected Files + +| File | Action | Rough size | +|------|--------|-----------| +| `src/cli/loading.ts` | create | ~40 lines (helper + `isSpinnerEnabled` + frame array) | +| `src/cli/loading.test.ts` | create | ~80 lines (5–7 tests, env-var setup/teardown, fake stream) | +| `src/cli/run.ts` | modify | 1 line wrap at line 39 + 1 import | +| `src/cli/vod.ts` | modify | 1 line wrap at line 40 + 1 import | +| `tests/vod-smoke.test.ts` OR `tests/loading-smoke.test.ts` | modify or create | ~15 lines for the redirected-pipeline assertion | + +Total: ~140 lines added, 2 lines modified, 2 imports added. Well within a single PR's review budget. + +## House Rules Compliance + +- **R1** (no throw in domain) — N/A; `loading.ts` is presentation. `withLoading` does not throw on its own; if `fn` rejects, the rejection propagates unchanged. Cleanup runs in `finally` regardless. +- **R2** (no `class` outside `src/tui/`) — Respected. Helper is a plain function. +- **R3** (ports have no callbacks) — Respected. `withLoading` is NOT a port; it is a CLI presentation utility below the hexagonal boundary. The `fn` parameter is a thunk for async execution, not an event-handler interface. R3 explicitly targets cross-layer interfaces (`BibleRepository` style). +- **R4** (Zod stays in `src/api/`) — N/A; no validation involved. +- **R5** (errors as discriminated unions) — Respected. No new error types introduced; existing `getPassage` Result flows through. +- **R6** (branded IDs via single factory) — N/A. +- **R7** (no conditional/mapped/template-literal types in domain/application) — N/A; `loading.ts` is presentation. Helper signature uses a single generic `T`. +- **R8** (TUI business state in `useReducer`) — N/A; CLI has no React state. +- **R9** (no `useEffect` for business logic) — N/A; CLI is imperative. The spinner uses `setInterval` directly in the helper, which is the correct primitive for CLI animation. +- **R10** (action names past-tense) — N/A; no actions involved. +- **R11** (no decorators) — Respected. +- **R12** (async data fns return `Promise>`) — Respected. `withLoading` is a presentation wrapper; the `T` it returns is whatever `fn` returns, which in the consumer call sites is `Result`. The wrapper does not re-shape the Result — it is transparent to it. + +Exceptions called out: none. + +## Out of Scope + +- Loading state for any future TUI screens (this is CLI-only; TUI uses OpenTUI/React and will need a different mechanism) +- Progress percentage or determinate progress bars (indeterminate spinner only) +- Custom themes or user-configurable spinner frames +- Multi-line layouts, gradient frames, or any fancy rendering +- A `--no-spinner` flag (`NO_COLOR` and `isTTY` already provide opt-out) +- Refactoring `isColorEnabled` to share implementation with `isSpinnerEnabled` (duplication is intentional today; revisit if a third gate appears) +- Spinner for any non-`getPassage` operation (other awaits in the codebase are sub-100ms and do not need feedback) +- Replacing the hand-rolled spinner with `ora` or another library +- Bun fake-timer infrastructure (the injectable `interval` parameter is the v1 strategy) + +## Open Questions + +None. All four exploration questions are decided above. diff --git a/openspec/changes/cli-loading-state/spec.md b/openspec/changes/cli-loading-state/spec.md new file mode 100644 index 0000000..7545213 --- /dev/null +++ b/openspec/changes/cli-loading-state/spec.md @@ -0,0 +1,258 @@ +# Spec — cli-loading-state + +## Capability + +CLI presentation — loading indicators. This change introduces the `loading` sub-module to the CLI adapter layer (`src/cli/`), a peer of `ansi.ts`. No existing capability spec exists for `cli`; this is the first delta. It extends the observable behavior of **run** (`src/cli/run.ts`) and **vod** (`src/cli/vod.ts`) — both previously specced as part of `verbum-vod`. + +--- + +## Requirements + +### REQ-1: withLoading signature + +**Statement**: `src/cli/loading.ts` SHALL export a generic function with the exact signature: + +```ts +export function withLoading( + stream: NodeJS.WriteStream, + fn: () => Promise, + options?: { interval?: number } +): Promise +``` + +**Rationale**: Locks the public API so call sites in `run.ts` and `vod.ts` have a single unambiguous contract to target. + +**Acceptance**: +- Given `src/cli/loading.ts` is imported +- When a caller invokes `withLoading(process.stderr, () => somePromise)` +- Then the TypeScript compiler accepts the call without type errors and `T` is inferred from `somePromise`'s resolved type + +--- + +### REQ-2: isSpinnerEnabled signature and export + +**Statement**: `src/cli/loading.ts` SHALL export a function `isSpinnerEnabled(stream: NodeJS.WriteStream): boolean` that is the sole gate for spinner activation. + +**Rationale**: Named export with a distinct responsibility from `isColorEnabled` — keeps the two concerns independently evolvable. + +**Acceptance**: +- Given `src/cli/loading.ts` is imported +- When `isSpinnerEnabled` is called with a stream object +- Then it returns a `boolean` without throwing + +--- + +### REQ-3: isSpinnerEnabled precedence chain + +**Statement**: `isSpinnerEnabled` SHALL evaluate the following conditions in order and return at the first match: + +| Priority | Condition | Return | +|----------|-----------|--------| +| 1 | `process.env.NO_COLOR` is a non-empty string | `false` | +| 2 | `process.env.FORCE_COLOR` is `"0"` or `"false"` | `false` | +| 3 | `process.env.FORCE_COLOR` is any other non-empty string | `true` | +| 4 | `stream.isTTY === true` | `true` | +| 5 | fallback | `false` | + +**Rationale**: Mirrors the `isColorEnabled` precedence in `ansi.ts` so that `NO_COLOR=1` suppressess both color and spinner with a single env variable, satisfying the stated success criterion. + +**Acceptance**: +- Given `NO_COLOR` is set to any non-empty string +- When `isSpinnerEnabled` is called with a TTY-true stream +- Then it returns `false` + +- Given `FORCE_COLOR` is `"0"` +- When `isSpinnerEnabled` is called with a TTY-true stream +- Then it returns `false` + +- Given `FORCE_COLOR` is `"1"` and `NO_COLOR` is unset +- When `isSpinnerEnabled` is called with a TTY-false stream +- Then it returns `true` + +- Given no `NO_COLOR` or `FORCE_COLOR` env vars are set +- When `isSpinnerEnabled` is called with a stream where `isTTY` is `undefined` or `false` +- Then it returns `false` + +--- + +### REQ-4: Spinner frames and interval + +**Statement**: When active, `withLoading` SHALL cycle through exactly these 10 Braille frames in order — `["⠋","⠙","⠹","⠸","⠼","⠴","⠦","⠧","⠇","⠏"]` — writing one frame per tick at a default interval of 80 ms. The frame display text SHALL be the frame character alone (no label, no color escapes). + +**Rationale**: Locks the visual output so tests can assert exact write content. Frame-only eliminates line-wrap risk and avoids vocabulary that could become stale. + +**Acceptance**: +- Given a TTY-true fake stream with a write-capture function +- When `withLoading` is called with `options: { interval: 1 }` and a `fn` that resolves after at least one tick +- Then the capture function is called with a write containing exactly one of the 10 Braille characters preceded by `\r` +- And the sequence of frames across ticks cycles through the array in index order, wrapping at index 10 + +--- + +### REQ-5: No-op when isSpinnerEnabled returns false + +**Statement**: When `isSpinnerEnabled(stream)` returns `false`, `withLoading` SHALL call `fn()`, await its result, and return it — without writing any bytes to `stream`. + +**Rationale**: Guarantees that piped, redirected, and CI invocations are byte-identical to the pre-change behavior. + +**Acceptance**: +- Given a fake stream with `isTTY: false` and a write spy +- When `withLoading(fakeStream, () => Promise.resolve(42))` is called and awaited +- Then the write spy is never called +- And the return value is `42` + +--- + +### REQ-6: Spinner write target is stderr only + +**Statement**: `withLoading` SHALL only write spinner frames and cleanup bytes to the `stream` argument it receives. It SHALL NOT write to `process.stdout` or any other global stream. + +**Rationale**: Stdout must remain pristine so that `verbum john 3:16 | jq` and `> out.txt` pipelines are unaffected. + +**Acceptance**: +- Given `process.stdout.write` is monkey-patched to capture calls +- When `withLoading(process.stderr, fn)` is called and awaited +- Then `process.stdout.write` is never called during or after the spinner + +--- + +### REQ-7: Cleanup before fn result propagates + +**Statement**: When `isSpinnerEnabled(stream)` returns `true`, `withLoading` SHALL, in a `finally` block: (1) call `clearInterval` on the spinner handle, then (2) write `"\r" + " ".repeat(frameWidth) + "\r"` to `stream` — where `frameWidth` is the character length of one frame (1 for a single Braille glyph). Both steps SHALL complete before the resolved value or rejection from `fn` propagates to the caller. + +**Rationale**: Ensures the spinner line is erased before any stdout output from the caller lands on the terminal, and prevents interval leaks into subsequent test cases. + +**Acceptance**: +- Given a TTY-true fake stream with a write spy, and `options: { interval: 1 }` +- When `withLoading` is called with a `fn` that resolves after a short delay +- Then after `await withLoading(...)` returns, the last write recorded by the spy is `"\r \r"` (carriage return + one space + carriage return) + +- Given a TTY-true fake stream and `options: { interval: 1 }` +- When `withLoading` is called with a `fn` that rejects +- Then the cleanup write (`"\r \r"`) is still written to the stream before the rejection propagates + +--- + +### REQ-8: Result transparency (R12 compliance) + +**Statement**: `withLoading` SHALL return exactly what `fn()` resolves to — including `Result` shapes — without wrapping, unwrapping, or transforming the value. If `fn()` rejects, `withLoading` SHALL re-throw the same rejection unchanged. + +**Rationale**: R12 requires async data functions to return `Promise>`. The call sites pass `() => getPassage(...)`, which already returns `Promise>`. The wrapper must not alter that shape. + +**Acceptance**: +- Given `fn` is `() => Promise.resolve({ ok: true, value: "verse" })` +- When `await withLoading(stream, fn)` resolves +- Then the returned value is `{ ok: true, value: "verse" }` with no additional wrapping + +- Given `fn` is `() => Promise.reject(new Error("network"))` +- When `withLoading(stream, fn)` is awaited inside a try/catch +- Then the caught error is the same `Error("network")` instance + +--- + +### REQ-9: run.ts integration + +**Statement**: `src/cli/run.ts` SHALL wrap its `getPassage(repo, ref)` call with `withLoading(process.stderr, () => getPassage(repo, ref))`. The import of `withLoading` SHALL come from `../cli/loading` (or the equivalent relative path). No other lines in `run.ts` SHALL change. + +**Rationale**: Delivers the spinner for the primary `verbum ` command path. + +**Acceptance**: +- Given a fake `process.stderr` with `isTTY: true` is injected (or the process is run in a TTY context) +- When `verbum john 3:16` runs +- Then at least one spinner frame is written to stderr before stdout receives the verse + +- Given `process.stderr.isTTY` is `false` +- When `verbum john 3:16` runs +- Then stderr receives zero bytes from the spinner (verse still appears on stdout) + +--- + +### REQ-10: vod.ts integration + +**Statement**: `src/cli/vod.ts` SHALL wrap its `getPassage(repo, ref)` call with `withLoading(process.stderr, () => getPassage(repo, ref))`. The import and wrapping pattern SHALL be identical to the one used in `run.ts` (REQ-9). + +**Rationale**: Delivers the spinner for the `verbum vod` command path on the same terms as the primary command. + +**Acceptance**: +- Given `process.stderr.isTTY` is `false` +- When `verbum vod` runs +- Then stderr receives zero bytes from the spinner (verse still appears on stdout) + +--- + +### REQ-11: No new dependencies + +**Statement**: `package.json` SHALL NOT gain any new entries in `dependencies` or `devDependencies` as a result of this change. + +**Rationale**: verbum has a strict no-utility-dep posture; a hand-rolled 40-line helper does not meet the threshold for a library dependency. + +**Acceptance**: +- Given the change is applied +- When `git diff HEAD -- package.json` is inspected +- Then no lines are added to `dependencies` or `devDependencies` + +--- + +### REQ-12: Unit test coverage surface + +**Statement**: `src/cli/loading.test.ts` SHALL include tests covering at minimum: + +| # | Scenario | +|---|----------| +| T1 | `isTTY=false` stream → no writes, fn result returned | +| T2 | `isTTY=true` stream, `interval:1` → at least one frame write occurs | +| T3 | `isTTY=true` stream, `interval:1` → last write is the cleanup sequence `"\r \r"` | +| T4 | `fn` rejects → cleanup write still occurs; rejection propagates | +| T5 | `NO_COLOR` set → no writes to stream | +| T6 | `FORCE_COLOR="0"` set → no writes to stream | +| T7 | `FORCE_COLOR="1"` and `isTTY=false` → writes occur (FORCE_COLOR wins) | + +Each test MUST use a fake stream object (`{ isTTY: boolean, write: spy }`) — no monkey-patching of globals. Env-var mutation MUST be restored in `afterEach`. + +**Rationale**: Establishes project convention for unit-testing animated CLI helpers without fake timers or global pollution. + +**Acceptance**: +- Given `bun test src/cli/loading.test.ts` runs +- Then all tests pass and Bun reports zero open handle warnings + +--- + +### REQ-13: Smoke test — no stderr bytes in non-TTY context + +**Statement**: The smoke test suite (either `tests/vod-smoke.test.ts` extended or a new `tests/loading-smoke.test.ts`) SHALL include an assertion that when `process.stderr.isTTY` is `false`, zero bytes from spinner logic are written to stderr during a `withLoading`-wrapped call. + +**Rationale**: Provides end-to-end proof that piped and redirected invocations remain byte-identical to the pre-change baseline. + +**Acceptance**: +- Given `process.stderr.write` is monkey-patched in a `try/finally` block +- And the fake stderr has `isTTY: false` +- When `withLoading` is called with a fast-resolving `fn` (no real network) +- Then the spy records zero calls attributed to the spinner + +--- + +## Out of Scope + +| Item | Reason | +|------|--------| +| TUI loading state | Different mechanism via OpenTUI/React; separate future change | +| Determinate progress bars | Indeterminate spinner only in v1 | +| Custom themes or user-configurable frames | No config layer exists; frame array is internal | +| `--no-spinner` CLI flag | `NO_COLOR` + `isTTY` already cover all opt-out cases | +| Refactoring `isColorEnabled` to share code with `isSpinnerEnabled` | Intentional duplication; revisit if a third gate appears | +| Spinner for non-`getPassage` operations | Other awaits are sub-100ms; no user-visible gap | +| Replacing hand-rolled with `ora` or similar | Below the library threshold (~40 lines, zero maintenance burden) | +| Bun fake-timer infrastructure | Injectable `interval` parameter is the v1 strategy | +| ANSI clear-line escape (`\x1b[2K`) | `\r` + spaces is universally safe and sufficient | +| Accent color on spinner frame | Accent is reserved for structural signals in `renderPassage` | + +--- + +## Spec-Level Risks + +| Risk | Status | +|------|--------| +| `setInterval` interval fires before `fn` resolves in unit tests | Mitigated: tests inject `options: { interval: 1 }` for tick-sensitive cases; smoke tests use fast-resolving `fn` so interval never fires before `finally` | +| Bun concurrent test runner and global monkey-patching | Mitigated: follow existing pattern of serializing via `describe` blocks; unit tests use fake streams (no globals) | +| `frameWidth` assumption (1 char per Braille glyph) | Braille Unicode block glyphs are single code points and single columns in all modern terminals; no special handling needed | +| `NO_COLOR`/`FORCE_COLOR` env pollution between tests | Mitigated by `afterEach` restore pattern established in `ansi.test.ts` | diff --git a/openspec/changes/cli-loading-state/tasks.md b/openspec/changes/cli-loading-state/tasks.md new file mode 100644 index 0000000..06dd344 --- /dev/null +++ b/openspec/changes/cli-loading-state/tasks.md @@ -0,0 +1,214 @@ +# Tasks — cli-loading-state + +## TL;DR + +13 tasks across 4 commits (C1–C4). Strict TDD mode: tests ship in their own commit +before the implementation commit. Net diff ~155 lines (logic) + 2 modified lines. +Single PR — logic surface is well under the 400-line budget. + +--- + +## Phase 1 — Red (failing tests) + +### C1 — `test(cli): add failing unit tests for loading module` + +Files: `src/cli/loading.test.ts` (NEW, ~80 lines) + +--- + +- [ ] **T-01 — Create `src/cli/loading.test.ts` with all unit test groups** + - REQ: REQ-2, REQ-3, REQ-4, REQ-5, REQ-6, REQ-7, REQ-8, REQ-12 + - Files: `src/cli/loading.test.ts` + - Rationale: Write every test described in the design before the implementation exists. All tests must fail at this point (import will fail entirely until loading.ts is created — that is expected and counts as red). + - Checklist: + - [ ] Import: `import { withLoading, isSpinnerEnabled, SPINNER_FRAMES } from "./loading";` + - [ ] Define `fakeStream(isTTY)` factory returning `{ stream: NodeJS.WriteStream, writes: string[] }` — pattern from `ansi.test.ts` + - [ ] **Group 1 — `isSpinnerEnabled` truth table** (cases S1–S8 from design): + - [ ] S1: TTY, no env → `true` + - [ ] S2: pipe, no env → `false` + - [ ] S3: TTY, `NO_COLOR="1"` → `false` + - [ ] S4: TTY, `NO_COLOR=""` (empty) → `true` (empty = no opinion) + - [ ] S5: pipe, `FORCE_COLOR="1"` → `true` (override) + - [ ] S6: TTY, `FORCE_COLOR="0"` → `false` + - [ ] S7: TTY, `FORCE_COLOR="false"` → `false` + - [ ] S8: pipe, `NO_COLOR="1"` + `FORCE_COLOR="1"` → `false` (NO_COLOR wins) + - [ ] Use `beforeEach`/`afterEach` env-var save/restore pattern cloned from `ansi.test.ts` + - [ ] **Group 2 — `withLoading` TTY-false no-op**: + - [ ] T-NOOP-1: `isTTY=false`, fn returns `42`, `interval: 1_000_000` → no writes, result `42` (REQ-5) + - [ ] T-NOOP-2: `isTTY=false`, fn returns `{ ok: true, value: "x" }` → passes through with referential equality (REQ-8) + - [ ] **Group 3 — `withLoading` TTY-true renders + cleans up**: + - [ ] T-TTY-1: `isTTY=true`, `interval: 1_000_000`, fn resolves `7` → `writes[0] === "\r⠋"`, `writes[last] === "\r \r"`, result `7` (REQ-4, REQ-6, REQ-7) + - [ ] T-TTY-2: `isTTY=true`, `interval: 1`, fn awaits microtask → cleanup is still last write (REQ-7 survival under real tick) + - [ ] **Group 4 — `withLoading` rejection path**: + - [ ] T-REJECT-1: `isTTY=true`, fn rejects `new Error("nope")` → `expect(...).rejects.toThrow("nope")`, `writes[last] === "\r \r"` after settle (REQ-7, REQ-8) + - [ ] **Group 5 — frame array shape**: + - [ ] T-FRAMES-1: `SPINNER_FRAMES.length === 10` (REQ-4) + - [ ] T-FRAMES-2: every frame is a single-grapheme string (`frame.length === 1`) (REQ-4) + - **Acceptance**: `bun test src/cli/loading.test.ts` — all tests FAIL with import error (red confirmed) + +--- + +## Phase 2 — Green (implementation) + +### C2 — `feat(cli): implement loading module — spinner + isSpinnerEnabled` + +Files: `src/cli/loading.ts` (NEW, ~50 lines) + +--- + +- [ ] **T-02 — Create `src/cli/loading.ts` with exports and types** + - REQ: REQ-1, REQ-2, REQ-11 + - Files: `src/cli/loading.ts` + - Rationale: Export the exact type-level surface locked by REQ-1 and REQ-2. TypeScript compiler must accept `withLoading(process.stderr, () => somePromise)` without errors and infer `T`. + - Checklist: + - [ ] Export `SPINNER_FRAMES` — 10-element `as const` Braille tuple: `["⠋","⠙","⠹","⠸","⠼","⠴","⠦","⠧","⠇","⠏"]` + - [ ] Export `SpinnerFrame = (typeof SPINNER_FRAMES)[number]` + - [ ] Export `WithLoadingOptions = { readonly interval?: number }` + - [ ] Export stub stubs `isSpinnerEnabled` and `withLoading` with correct signatures (can be stubs at this step — next task fills bodies) + - [ ] No `package.json` changes (REQ-11) + +- [ ] **T-03 — Implement `isSpinnerEnabled` precedence chain** + - REQ: REQ-2, REQ-3 + - Files: `src/cli/loading.ts` + - Rationale: Exactly mirrors `isColorEnabled` in `ansi.ts`. Precedence: NO_COLOR (non-empty) → false; FORCE_COLOR "0"/"false" → false; FORCE_COLOR (other non-empty) → true; isTTY === true → true; fallback → false. + - Checklist: + - [ ] Step 1: `if (process.env.NO_COLOR) return false` + - [ ] Step 2: read `process.env.FORCE_COLOR`; if present, lowercase it: `"0"` or `"false"` → `false`; else → `true` + - [ ] Step 3: fallback `return stream.isTTY === true` + - [ ] No shared utility with `isColorEnabled` — intentional duplication (design decision) + +- [ ] **T-04 — Implement `withLoading` control flow** + - REQ: REQ-1, REQ-4, REQ-5, REQ-6, REQ-7, REQ-8 + - Files: `src/cli/loading.ts` + - Rationale: Two paths — (a) TTY-false: return `fn()` directly, zero writes; (b) TTY-true: initial frame write → setInterval → try/await fn → finally clearInterval + cleanup write. Cleanup must be `"\r" + " ".repeat(1) + "\r"` — erase exactly the frame column. + - Checklist: + - [ ] TTY-false path: `if (!isSpinnerEnabled(stream)) return fn()` — no try/finally, no writes + - [ ] TTY-true path: write `"\r" + SPINNER_FRAMES[0]` before starting interval (initial render before awaiting fn) + - [ ] Start `setInterval` at `options?.interval ?? 80` — each tick: `stream.write("\r" + SPINNER_FRAMES[i % SPINNER_FRAMES.length]); i++` + - [ ] `try { return await fn() } finally { clearInterval(handle); stream.write("\r \r") }` + - [ ] Interval handle typed as `ReturnType` (not `NodeJS.Timeout`) + - [ ] Never write to `process.stdout` or any stream other than the passed `stream` + - [ ] `fn` rejection propagates unchanged — do NOT catch or convert to Result (R1 spirit) + - **Acceptance**: `bun test src/cli/loading.test.ts` — all ~14 tests pass, zero open handle warnings (green confirmed) + +--- + +## Phase 3 — Integration + +### C3 — `feat(cli): wrap getPassage with withLoading in run.ts and vod.ts` + +Files: `src/cli/run.ts` (MODIFY), `src/cli/vod.ts` (MODIFY) + +--- + +- [ ] **T-05 — Integrate `withLoading` into `run.ts`** + - REQ: REQ-9 + - Files: `src/cli/run.ts` + - Rationale: Deliver the spinner for the primary `verbum ` command path. Exactly one line changes plus one import — no other lines touch (REQ-9 constraint). + - Checklist: + - [ ] Add `import { withLoading } from "@/cli/loading";` after the existing `@/cli/render` import + - [ ] Change line 39: `await getPassage(repo, refResult.value)` → `await withLoading(process.stderr, () => getPassage(repo, refResult.value))` + - [ ] No other lines in `run.ts` change + +- [ ] **T-06 — Integrate `withLoading` into `vod.ts`** + - REQ: REQ-10 + - Files: `src/cli/vod.ts` + - Rationale: Deliver the spinner for `verbum vod`. Pattern identical to REQ-9 (same import path, same wrapping shape). + - Checklist: + - [ ] Add `import { withLoading } from "@/cli/loading";` after the existing `@/cli/render` import + - [ ] Change line 40: `await getPassage(repo, ref)` → `await withLoading(process.stderr, () => getPassage(repo, ref))` + - [ ] No other lines in `vod.ts` change + - **Acceptance**: `bun test` full suite passes; layer audit clean (no new dependencies in `package.json`) + +--- + +## Phase 4 — Smoke + +### C4 — `test(cli): extend vod-smoke with loading-state assertion` + +Files: `tests/vod-smoke.test.ts` (MODIFY — append one `describe` block) + +--- + +- [ ] **T-07 — Extend `tests/vod-smoke.test.ts` with no-spinner-in-pipe smoke assertion** + - REQ: REQ-13 + - Files: `tests/vod-smoke.test.ts` + - Rationale: End-to-end proof that piped/redirected invocations get zero spinner bytes on stderr. Uses `NO_COLOR=1` (safe save/restore in finally) rather than mutating `process.stderr.isTTY`. Extends the existing file — no new smoke file (design decision to avoid harness duplication). + - Checklist: + - [ ] Append `describe("smoke — verbum vod loading state is invisible to redirected pipelines", ...)` block + - [ ] Save `process.env.NO_COLOR` before mutation; restore in `finally` + - [ ] Monkey-patch `process.stderr.write` inside the describe block; restore in `finally` + - [ ] Call `await runVod(fixed, stubRepo)` with a deterministic fixed date and the existing `stubRepo` + - [ ] Assert none of the 10 Braille frames appear in captured stderr + - [ ] Assert `"\r"` does not appear in captured stderr + - [ ] No new test files — only extend `tests/vod-smoke.test.ts` + - **Acceptance**: `bun test tests/vod-smoke.test.ts` passes; Bun reports zero open handle warnings + +--- + +## Phase 5 — Verify + +- [ ] **T-08 — Run full test suite and confirm zero regressions** + - REQ: REQ-11, REQ-12, REQ-13 + - Files: none (read-only verification) + - Rationale: Final gate before PR — all pre-existing tests must continue to pass; no new dependencies in `package.json`. + - Checklist: + - [ ] `bun test` — all tests pass + - [ ] Confirm `package.json` is unchanged (REQ-11) + - [ ] Confirm `bun test src/cli/loading.test.ts` exits with zero and no open handle warnings + - [ ] Confirm `bun test tests/vod-smoke.test.ts` exits with zero + +--- + +## Dependency Order + +``` +T-01 (red tests) + └─ T-02 (types + exports) + └─ T-03 (isSpinnerEnabled impl) + └─ T-04 (withLoading impl — green) + ├─ T-05 (run.ts integration) + └─ T-06 (vod.ts integration) + └─ T-07 (smoke extension) + └─ T-08 (verify) +``` + +T-05 and T-06 can be done in either order within C3 — they touch different files and have no shared dependency between them. + +--- + +## Commit Map + +| Commit | Tasks | Files | TDD Phase | +|--------|-------|-------|-----------| +| C1 | T-01 | `src/cli/loading.test.ts` (NEW) | Red | +| C2 | T-02, T-03, T-04 | `src/cli/loading.ts` (NEW) | Green | +| C3 | T-05, T-06 | `src/cli/run.ts`, `src/cli/vod.ts` (MODIFY) | Integration | +| C4 | T-07, T-08 | `tests/vod-smoke.test.ts` (MODIFY) | Smoke + Verify | + +--- + +## House Rules Compliance (per task) + +| Task | Rules checked | +|------|---------------| +| T-01 | REQ-12: fake streams only, no global monkey-patch; `afterEach` env restore | +| T-02 | R11: no decorators; R1: transparent to Result; R5: no new error types | +| T-03 | R12: isSpinnerEnabled is the sole gate; intentional duplication (not R3 — not a port) | +| T-04 | R1: never throws; R12: `withLoading` transparent; R3 N/A (thunk ≠ port callback) | +| T-05 | REQ-9: only one line + one import change | +| T-06 | REQ-10: identical pattern to T-05 | +| T-07 | REQ-13: NO_COLOR save/restore in `finally`; no new test files | +| T-08 | REQ-11: `package.json` unchanged | + +--- + +## Review Workload Forecast + +| Field | Value | +|-------|-------| +| Estimated changed lines | ~155 (loading.ts ~50 + loading.test.ts ~80 + run.ts +2 + vod.ts +2 + smoke +20) | +| 400-line budget risk | Low | +| Chained PRs recommended | No | +| Decision needed before apply | No | +| Suggested PR structure | Single PR (4 ordered work-unit commits) | diff --git a/openspec/changes/cli-loading-state/verify-report.md b/openspec/changes/cli-loading-state/verify-report.md new file mode 100644 index 0000000..ee33557 --- /dev/null +++ b/openspec/changes/cli-loading-state/verify-report.md @@ -0,0 +1,77 @@ +# Verify Report — cli-loading-state + +## Summary + +**Status**: PASS +**Test results**: 99 passing, 0 failing +**Total findings**: 0 CRITICAL, 1 WARNING, 2 SUGGESTION + +--- + +## Requirement Coverage + +| REQ | Status | Evidence (file:line) | Notes | +|-----|--------|----------------------|-------| +| REQ-1 | Pass | `loading.ts:39–43` | Exact generic signature matches spec | +| REQ-2 | Pass | `loading.ts:17` | `isSpinnerEnabled` exported, returns boolean, never throws | +| REQ-3 | Pass | `loading.ts:18–32` | `NO_COLOR` → `FORCE_COLOR "0"/"false"` → `FORCE_COLOR` truthy → `isTTY` → `false`. Correct precedence. | +| REQ-4 | Pass | `loading.ts:5–7,52–58`; `loading.test.ts:127` | All 10 Braille frames; `\r+frame` format; default 80 ms; T-FRAMES-1/2 assert count and grapheme width | +| REQ-5 | Pass | `loading.ts:44–46`; `loading.test.ts:98–116` | No writes when `isTTY=false`; fn result returned unchanged | +| REQ-6 | Pass | `loading.ts:39–66` | `stream` is the only write target; no `process.stdout.write` inside implementation | +| REQ-7 | Pass | `loading.ts:60–65`; `loading.test.ts:129,154` | `clearInterval` then `"\r \r"` in `finally`; T-TTY-1 and T-REJECT-1 both confirm | +| REQ-8 | Pass | `loading.ts:61` | `return await fn()` with no wrapping; rejection re-thrown via unhandled `finally` | +| REQ-9 | Pass | `run.ts:10,40` | `withLoading` wraps `getPassage` at line 40; import from `@/cli/loading` | +| REQ-10 | Pass | `vod.ts:13,41` | Identical pattern; import from `@/cli/loading` | +| REQ-11 | Pass | `git diff HEAD -- package.json`: 0 deps/devDeps lines added | Metadata fields only (description, keywords, etc.) | +| REQ-12 | Pass | `loading.test.ts:1–171` | T1(T-NOOP-1), T2(T-TTY-2), T3(T-TTY-1), T4(T-REJECT-1), T5(S3), T6(S6), T7(S5) all present; fake streams throughout; `afterEach` restores env | +| REQ-13 | Partial | `vod-smoke.test.ts:129–163` | Smoke uses `fakeStderr` with `isTTY:false` + `NO_COLOR=1` env var. Does not exercise the pure-TTY gate independently in smoke context (covered by unit test T-NOOP-1). See WARNING W-1. | + +--- + +## House Rules Compliance + +| Rule | Status | Notes | +|------|--------|-------| +| R1/R5 | Pass | `withLoading` is not a Result producer; `fn` rejection propagates unchanged via `finally`; no throw added | +| R2 | Pass | No classes in `loading.ts` or the two modified files; pure function exports only | +| R3 | Pass | `withLoading` lives in `src/cli/` (adapter layer), not in any ports file | +| R7 | Pass | No conditional/mapped/template-literal types. `WithLoadingOptions` is a plain object type | +| R11 | Pass | No decorators anywhere in new or modified files | +| simplify — no new abstractions | Pass | Only `withLoading`, `isSpinnerEnabled`, `SPINNER_FRAMES` exported — nothing extra | +| `isSpinnerEnabled` duplication | Pass | Intentional mirror of `isColorEnabled`; comment at `loading.ts:15` documents the decision | + +--- + +## Commit Discipline + +| Commit | SHA | Conforms? | Notes | +|--------|-----|-----------|-------| +| C1 | `5d0fd96` | Yes | Only `loading.test.ts` added (171 lines); no `loading.ts` in this commit; `test(cli):` prefix | +| C2 | `d477179` | Yes | Only `loading.ts` added (66 lines); turns C1 tests green; `feat(cli):` prefix | +| C3 | `74e5f50` | Yes | Only `run.ts` and `vod.ts` modified (+3 lines each); no test changes; `feat(cli):` prefix | +| C4 | `f77ab54` | Yes | Only `vod-smoke.test.ts` modified (+39 lines); no implementation changes; `test(cli):` prefix | +| Co-Authored-By | Pass | No AI attribution in any of the 4 commits | + +--- + +## Findings + +### CRITICAL + +None. + +### WARNING + +**W-1 (REQ-13 / Smoke coverage gap)**: The smoke test validates the `NO_COLOR=1` env-var path with a `isTTY:false` fake stream. Both gates overlap — if the isTTY=false branch alone started writing (regression), the smoke would still pass because `NO_COLOR=1` is also active. The pure isTTY gate is covered by unit test T-NOOP-1, so there is no correctness defect, but the smoke does not independently exercise the TTY gate. + +### SUGGESTION + +**S-1 (REQ-4 / initial-frame-on-entry not locked in spec)**: `T-TTY-1` asserts `writes[0] === "\r⠋"` — a synchronous frame write before `setInterval`. The implementation does this (loading.ts:52–53). REQ-4 does not explicitly mandate a synchronous initial frame, only per-tick frames. A future refactor removing the initial write would break the test without violating the written spec. The spec should be hardened: "withLoading SHALL write the first frame synchronously before starting the interval." + +**S-2 (REQ-6 / no stdout-spy test)**: REQ-6 requires `process.stdout.write` is never called during a spinner. This is structurally guaranteed (`loading.ts` never references `process.stdout`), but there is no automated regression guard. A one-line spy test in `loading.test.ts` would close this gap permanently. + +--- + +## Recommendation + +**Archive** — all 13 REQs are met, test suite is 99/99, TDD discipline was followed across all 4 commits, and house rules are clean. W-1 is a coverage gap in the smoke test (not a defect). S-1 and S-2 are spec/test surface improvements suited for a follow-up task. diff --git a/src/cli/loading.test.ts b/src/cli/loading.test.ts new file mode 100644 index 0000000..11f1bfe --- /dev/null +++ b/src/cli/loading.test.ts @@ -0,0 +1,171 @@ +// src/cli/loading.test.ts — unit tests for loading module (withLoading, isSpinnerEnabled). +// Pure tests — no terminal allocation, no real IO. Fake streams only. +// process.env is mutated and restored via beforeEach/afterEach per ansi.test.ts convention. + +import { describe, it, expect, beforeEach, afterEach } from "bun:test"; +import { withLoading, isSpinnerEnabled, SPINNER_FRAMES } from "./loading"; + +// Fake-stream factory — returns a WriteStream-like object and captures all writes. +function fakeStream(isTTY: boolean): { stream: NodeJS.WriteStream; writes: string[] } { + const writes: string[] = []; + const stream = { + isTTY, + write(chunk: string | Uint8Array) { + writes.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); + return true; + }, + } as unknown as NodeJS.WriteStream; + return { stream, writes }; +} + +// ─── Group 1: isSpinnerEnabled truth table ──────────────────────────────────── + +describe("isSpinnerEnabled — truth table", () => { + let savedNoColor: string | undefined; + let savedForceColor: string | undefined; + + beforeEach(() => { + savedNoColor = process.env.NO_COLOR; + savedForceColor = process.env.FORCE_COLOR; + delete process.env.NO_COLOR; + delete process.env.FORCE_COLOR; + }); + + afterEach(() => { + if (savedNoColor === undefined) { + delete process.env.NO_COLOR; + } else { + process.env.NO_COLOR = savedNoColor; + } + if (savedForceColor === undefined) { + delete process.env.FORCE_COLOR; + } else { + process.env.FORCE_COLOR = savedForceColor; + } + }); + + it("S1 TTY no env → true", () => { + const { stream } = fakeStream(true); + expect(isSpinnerEnabled(stream)).toBe(true); + }); + + it("S2 pipe no env → false", () => { + const { stream } = fakeStream(false); + expect(isSpinnerEnabled(stream)).toBe(false); + }); + + it("S3 TTY NO_COLOR=1 → false", () => { + process.env.NO_COLOR = "1"; + const { stream } = fakeStream(true); + expect(isSpinnerEnabled(stream)).toBe(false); + }); + + it('S4 TTY NO_COLOR="" → true (empty = no opinion)', () => { + process.env.NO_COLOR = ""; + const { stream } = fakeStream(true); + expect(isSpinnerEnabled(stream)).toBe(true); + }); + + it("S5 pipe FORCE_COLOR=1 → true (FORCE_COLOR wins over isTTY=false)", () => { + process.env.FORCE_COLOR = "1"; + const { stream } = fakeStream(false); + expect(isSpinnerEnabled(stream)).toBe(true); + }); + + it("S6 TTY FORCE_COLOR=0 → false (npm convention)", () => { + process.env.FORCE_COLOR = "0"; + const { stream } = fakeStream(true); + expect(isSpinnerEnabled(stream)).toBe(false); + }); + + it("S7 TTY FORCE_COLOR=false → false (npm convention)", () => { + process.env.FORCE_COLOR = "false"; + const { stream } = fakeStream(true); + expect(isSpinnerEnabled(stream)).toBe(false); + }); + + it("S8 NO_COLOR=1 + FORCE_COLOR=1 → false (NO_COLOR wins, no-color.org)", () => { + process.env.NO_COLOR = "1"; + process.env.FORCE_COLOR = "1"; + const { stream } = fakeStream(false); + expect(isSpinnerEnabled(stream)).toBe(false); + }); +}); + +// ─── Group 2: No-op when spinner is disabled ────────────────────────────────── + +describe("withLoading — no-op when isTTY=false", () => { + it("T-NOOP-1 isTTY=false, fn returns 42 → no writes, result 42", async () => { + const { stream, writes } = fakeStream(false); + let callCount = 0; + const result = await withLoading(stream, async () => { + callCount++; + return 42; + }, { interval: 1_000_000 }); + expect(result).toBe(42); + expect(writes).toHaveLength(0); + expect(callCount).toBe(1); + }); + + it("T-NOOP-2 isTTY=false, fn returns Result shape → passes through unchanged", async () => { + const { stream, writes } = fakeStream(false); + const value = { ok: true as const, value: "verse" }; + const result = await withLoading(stream, async () => value, { interval: 1_000_000 }); + expect(result).toBe(value); + expect(writes).toHaveLength(0); + }); +}); + +// ─── Group 3: TTY-true — renders frames and cleans up ──────────────────────── + +describe("withLoading — TTY=true renders spinner and cleans up", () => { + it("T-TTY-1 interval=1_000_000 → first write is \\r⠋, last write is cleanup, result 7", async () => { + const { stream, writes } = fakeStream(true); + const result = await withLoading(stream, async () => 7, { interval: 1_000_000 }); + expect(result).toBe(7); + // First write: initial frame before interval starts + expect(writes[0]).toBe("\r⠋"); // \r + first Braille frame + // Last write: cleanup erase + expect(writes[writes.length - 1]).toBe("\r \r"); + }); + + it("T-TTY-2 interval=1, fn awaits microtask → cleanup is still the last write", async () => { + const { stream, writes } = fakeStream(true); + await withLoading( + stream, + () => new Promise((resolve) => setTimeout(resolve, 5)), + { interval: 1 }, + ); + expect(writes[writes.length - 1]).toBe("\r \r"); + // At least the initial frame + at least one interval tick + cleanup + expect(writes.length).toBeGreaterThanOrEqual(2); + }); +}); + +// ─── Group 4: Rejection — cleanup still fires ──────────────────────────────── + +describe("withLoading — rejection propagates after cleanup", () => { + it("T-REJECT-1 isTTY=true, fn rejects → rejects with same error, cleanup is last write", async () => { + const { stream, writes } = fakeStream(true); + const err = new Error("nope"); + await expect( + withLoading(stream, () => Promise.reject(err), { interval: 1_000_000 }), + ).rejects.toThrow("nope"); + expect(writes[writes.length - 1]).toBe("\r \r"); + }); +}); + +// ─── Group 5: Frame shape ───────────────────────────────────────────────────── + +describe("SPINNER_FRAMES — shape invariants", () => { + it("T-FRAMES-1 has exactly 10 frames", () => { + expect(SPINNER_FRAMES).toHaveLength(10); + }); + + it("T-FRAMES-2 every frame is exactly 1 grapheme cluster", () => { + for (const frame of SPINNER_FRAMES) { + // Spread into Unicode code points — each Braille glyph is 1 code point. + expect([...frame]).toHaveLength(1); + } + }); +}); diff --git a/src/cli/loading.ts b/src/cli/loading.ts new file mode 100644 index 0000000..949aaed --- /dev/null +++ b/src/cli/loading.ts @@ -0,0 +1,66 @@ +// src/cli/loading.ts — TTY-aware loading spinner for CLI surfaces. +// Peer of ansi.ts — same env-var precedence chain, separate semantic concern. +// R1/R5: no Result usage; R2: plain functions; R11: HOF, not decorator. + +export const SPINNER_FRAMES = [ + "⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏", +] as const; + +export type SpinnerFrame = (typeof SPINNER_FRAMES)[number]; + +export type WithLoadingOptions = { + readonly interval?: number; // ms; default 80 +}; + +// isSpinnerEnabled mirrors isColorEnabled from ansi.ts — deliberate duplication +// for semantic separation (spinner can be disabled independently of color). +export function isSpinnerEnabled(stream: NodeJS.WriteStream): boolean { + const env = process.env; + const noColor = env.NO_COLOR; + + // NO_COLOR: any non-empty value disables (https://no-color.org). Empty/unset = no opinion. + if (typeof noColor === "string" && noColor.length > 0) return false; + + const force = env.FORCE_COLOR; + if (typeof force === "string" && force.length > 0) { + // npm convention: "0" and "false" disable; anything else enables. + const f = force.toLowerCase(); + if (f === "0" || f === "false") return false; + return true; + } + + return stream.isTTY === true; +} + +// withLoading — wraps an async fn with a Braille spinner on stream (stderr). +// TTY-false path: zero writes, zero overhead, fn result passed through unchanged. +// TTY-true path: initial frame written before interval, cleanup in finally. +// Result transparency: never wraps, unwraps, or catches fn's value (R12). +export async function withLoading( + stream: NodeJS.WriteStream, + fn: () => Promise, + options?: WithLoadingOptions, +): Promise { + if (!isSpinnerEnabled(stream)) { + return fn(); + } + + const frameWidth = 1; // All Braille frames are single-column glyphs. + let i = 0; + + // Write initial frame immediately — guarantees user sees spinner even if fn resolves fast. + stream.write("\r" + SPINNER_FRAMES[i % SPINNER_FRAMES.length]); + i++; + + const handle: ReturnType = setInterval(() => { + stream.write("\r" + SPINNER_FRAMES[i % SPINNER_FRAMES.length]); + i++; + }, options?.interval ?? 80); + + try { + return await fn(); + } finally { + clearInterval(handle); + stream.write("\r" + " ".repeat(frameWidth) + "\r"); + } +} diff --git a/src/cli/run.ts b/src/cli/run.ts index 202dc79..500e1d3 100644 --- a/src/cli/run.ts +++ b/src/cli/run.ts @@ -7,6 +7,7 @@ import { parseReference } from "@/domain/reference"; import { getPassage } from "@/application/get-passage"; import { createHelloAoBibleRepository } from "@/api/hello-ao-bible-repository"; import { renderParseError, renderRepoError, renderPassage } from "@/cli/render"; +import { withLoading } from "@/cli/loading"; import { isRepoError } from "@/domain/errors"; import { runVod } from "@/cli/vod"; @@ -36,7 +37,7 @@ export async function run(argv: string[]): Promise { } const repo = createHelloAoBibleRepository(); - const passageResult = await getPassage(repo, refResult.value); + const passageResult = await withLoading(process.stderr, () => getPassage(repo, refResult.value)); if (!passageResult.ok) { // AppError = ParseError | RepoError. ParseError was already handled above diff --git a/src/cli/vod.ts b/src/cli/vod.ts index 584bfb0..067fb15 100644 --- a/src/cli/vod.ts +++ b/src/cli/vod.ts @@ -10,6 +10,7 @@ import { VERSE_POOL } from "@/cli/verse-pool-data"; import { makeBookId } from "@/domain/book-id"; import { getPassage } from "@/application/get-passage"; import { renderParseError, renderRepoError, renderPassage } from "@/cli/render"; +import { withLoading } from "@/cli/loading"; import { createHelloAoBibleRepository } from "@/api/hello-ao-bible-repository"; import type { BibleRepository } from "@/application/ports/bible-repository"; import type { Reference } from "@/domain/reference"; @@ -37,7 +38,7 @@ export async function runVod( verses: { start: entry.verse, end: entry.verse }, }; - const passageResult = await getPassage(repo, ref); + const passageResult = await withLoading(process.stderr, () => getPassage(repo, ref)); if (!passageResult.ok) { // AppError = ParseError | RepoError. Pool entries passed makeBookId so the // expected failures from getPassage are RepoErrors. Narrow via the type diff --git a/tests/vod-smoke.test.ts b/tests/vod-smoke.test.ts index f47cdbe..21182fc 100644 --- a/tests/vod-smoke.test.ts +++ b/tests/vod-smoke.test.ts @@ -4,6 +4,7 @@ import { describe, it, expect } from "bun:test"; import { runVod } from "@/cli/vod"; +import { withLoading } from "@/cli/loading"; import { VERSE_POOL } from "@/cli/verse-pool-data"; import { pickVerseForDate } from "@/application/verse-pool"; import type { BibleRepository } from "@/application/ports/bible-repository"; @@ -122,3 +123,41 @@ describe("smoke — verbum vod happy path", () => { expect(a.length).toBeGreaterThan(0); }); }); + +// ─── Smoke: loading suppression under NO_COLOR=1 ───────────────────────────── + +describe("smoke — withLoading writes zero bytes to non-TTY stream (NO_COLOR=1)", () => { + it("no Braille frames and no \\r written to stderr when NO_COLOR=1", async () => { + const savedNoColor = process.env.NO_COLOR; + const stderrWrites: string[] = []; + + // Fake non-TTY stderr for withLoading — safe because isTTY:false is the pipe path. + const fakeStderr = { + isTTY: false as const, + write(chunk: string | Uint8Array) { + stderrWrites.push(typeof chunk === "string" ? chunk : Buffer.from(chunk).toString()); + return true; + }, + } as unknown as NodeJS.WriteStream; + + try { + process.env.NO_COLOR = "1"; + const fixed = new Date("2026-01-15"); + await withLoading(fakeStderr, () => runVod(fixed, stubRepo)); + } finally { + if (savedNoColor === undefined) { + delete process.env.NO_COLOR; + } else { + process.env.NO_COLOR = savedNoColor; + } + } + + const capturedStderr = stderrWrites.join(""); + // No Braille spinner characters should appear in stderr. + for (const frame of ["⠋", "⠙", "⠹", "⠸", "⠼", "⠴", "⠦", "⠧", "⠇", "⠏"]) { + expect(capturedStderr).not.toContain(frame); + } + // No carriage returns from spinner cleanup should appear either. + expect(capturedStderr).not.toContain("\r"); + }); +});