feat: extract CLI presenters, add --ndjson + color control (PRESENTER, v10.8.0) #25
Conversation
…, v10.8.0) Extract 9 render functions and emit() from bin/warp-graph.js into bin/presenters/ (json.js, text.js, index.js). Adds --ndjson flag for compact single-line JSON output, sanitizes _-prefixed internal keys from JSON/NDJSON payloads, and respects NO_COLOR/FORCE_COLOR/CI env vars for automatic ANSI stripping. Net -460 LOC in warp-graph.js.
|
@coderabbitai review please |
📝 WalkthroughWalkthroughIntroduces a unified CLI presenter system (new bin/presenters/*) that centralizes output rendering, adds NDJSON (--ndjson) and color-control behavior, extracts text renderers from warp-graph.js, updates warp-graph.js to call present(), and adds tests and docs. Version bumped to 10.8.0. Changes
Sequence DiagramsequenceDiagram
participant CLI as "CLI User"
participant Warp as "bin/warp-graph.js"
participant Presenter as "bin/presenters/index.js"
participant Formatter as "json.js / text.js"
participant Std as "stdout / stderr"
CLI->>Warp: run command (args)
Warp->>Warp: parse args, execute command
Warp->>Presenter: present(payload, {format, command, view})
alt payload.error
Presenter->>Std: renderError(payload) -> stderr
else format == "ndjson"
Presenter->>Formatter: sanitizePayload → compactStringify
Formatter->>Std: write single-line JSON
else format == "json"
Presenter->>Formatter: sanitizePayload → stableStringify
Formatter->>Std: write pretty JSON
else view present (svg/html)
Presenter->>Formatter: writeHtmlExport / handleFileExport
Formatter->>Std: write file path message
else text
Presenter->>Formatter: TEXT_RENDERERS[command](payload)
Formatter->>Std: write human-readable text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Actions performedReview triggered.
|
Release Preflight
If you tag this commit as |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (1)
43-55:⚠️ Potential issue | 🔴 Critical
bin/presenters/is missing from thefilesarray — published package will break at runtime.
bin/warp-graph.jsimports from./presenters/json.jsand./presenters/index.js, butbin/presenters/is not listed in thefilesarray. When published to npm, the presenters directory will be excluded from the tarball, causing aMODULE_NOT_FOUNDcrash on every CLI invocation.🐛 Proposed fix
"files": [ "bin/warp-graph.js", + "bin/presenters", "bin/git-warp", "src",bin/warp-graph.js (1)
91-98:⚠️ Potential issue | 🟡 Minor
CliOptionstypedef is missing thendjsonproperty.The
ndjsonfield is added tocreateDefaultOptions(line 237), parsed inconsumeBaseArg(line 267), and read inmain()(lines 2367, 2370, 2403), but it was not added to theCliOptionsJSDoc typedef. This will silently pass if the typedef isn't enforced, but it creates a documentation gap and may cause issues if stricter type checking is applied later.Proposed fix
/** * `@typedef` {Object} CliOptions * `@property` {string} repo * `@property` {boolean} json + * `@property` {boolean} ndjson * `@property` {string|null} view * `@property` {string|null} graph * `@property` {string} writer * `@property` {boolean} help */
🧹 Nitpick comments (3)
bin/presenters/index.js (1)
187-208: Potential null access whencommand === 'query'and_renderedAsciiis missing.Line 197 accesses
payload._renderedAsciiwithout a guard. If a query command reaches view mode without pre-rendered ASCII (e.g., due to an upstream bug), this would write"undefined\n"to stdout. A defensive check would be consistent with the guard pattern used inhandleFileExport(line 106).♻️ Suggested defensive guard
if (command === 'query') { - writeText(`${payload._renderedAscii}\n`, strip); + writeText(`${payload._renderedAscii ?? ''}\n`, strip); return; }test/bats/cli-ndjson.bats (1)
57-68: Minor: error test uses a relaxed line-count assertion (-le 2).Line 63 asserts
line_count -le 2rather than-eq 1. If NDJSON errors should produce exactly one JSON line, tightening this to-eq 1would catch regressions where extra output leaks through. The current assertion would pass even if a warning line is also emitted.test/unit/presenters/present.test.js (1)
117-120:CIenv-var branch may not be reached in this test.In typical test runners
process.stdout.isTTYisundefined/falsy, soshouldStripColor()returnstruefrom the!process.stdout.isTTYcheck before reaching theCIguard. The assertion passes coincidentally rather than exercising the intended branch. Consider stubbingisTTYso the CI-specific path is actually covered:Proposed fix
it('strips when CI is set', () => { + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }); process.env.CI = 'true'; expect(shouldStripColor()).toBe(true); + Object.defineProperty(process.stdout, 'isTTY', { value: undefined, configurable: true }); });
… test coverage - Add bin/presenters to package.json files array (critical: npm publish fix) - Add ndjson to CliOptions JSDoc typedef - Add ?? '' null guard on _renderedAscii in view query path - Tighten BATS ndjson error test to assert exactly 1 line - Stub isTTY in CI env test so CI branch is actually exercised - Add 3 regression tests (npm files, null guard, non-TTY strip)
Release Preflight
If you tag this commit as |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@bin/presenters/index.js`:
- Around line 34-57: The implementation of shouldStripColor() does not match its
docstring precedence: currently FORCE_COLOR being set to any non-empty value
short-circuits and ignores NO_COLOR; to fix, adjust the logic in
shouldStripColor() so NO_COLOR is evaluated before the FORCE_COLOR non-empty
branch (or alternatively update the docstring if FORCE_COLOR is intentionally
meant to override NO_COLOR) — locate the shouldStripColor function and either
move the NO_COLOR check above the FORCE_COLOR !== '' branch or change the
comment to state that FORCE_COLOR overrides NO_COLOR.
In `@test/unit/presenters/present.test.js`:
- Around line 117-120: The CI test is passing for the wrong reason because
shouldStripColor() checks !process.stdout.isTTY before process.env.CI; update
the test in present.test.js to explicitly set process.stdout.isTTY = true before
setting process.env.CI = 'true' (and restore both in afterEach) so the CI branch
in shouldStripColor() is exercised; likewise, when adding a test for the final
`return false` path, set process.stdout.isTTY = true and ensure process.env.CI
is unset (and restore afterward) so the test targets the intended branch.
🧹 Nitpick comments (3)
test/bats/cli-ndjson.bats (2)
57-67: Error test allows up to 2 lines but comment says "single-line JSON".Line 63 asserts
line_count -le 2while the comment on Line 60 says "stdout should be single-line JSON." If the contract is truly single-line NDJSON even for errors, tighten this to-eq 1. If the slack is intentional (e.g., an extra trailing newline), a brief comment explaining why would help.
34-43:_-prefixed key check is shallow — consistent withsanitizePayload, but worth noting.
Object.keys(obj)only inspects top-level keys. If nested objects ever carry_-prefixed keys, this test won't catch them. This matches the current shallowsanitizePayloadimplementation, so no issue today, but consider adding a recursive check if the sanitization contract expands.test/unit/presenters/present.test.js (1)
14-26: Monkey-patchingprocess.stdout.writeis reasonable here but considervi.spyOnfor cleaner teardown.Using
vi.spyOn(process.stdout, 'write').mockImplementation(...)would auto-restore inafterEachwithvi.restoreAllMocks()and avoids manual save/restore. Minor ergonomic nit.
…dence FORCE_COLOR!='' intentionally overrides NO_COLOR per the de facto standard. Updated JSDoc to document the full precedence chain and added a regression test for the FORCE_COLOR > NO_COLOR override.
Release Preflight
If you tag this commit as |
1 participant
Summary
emit()frombin/warp-graph.jsintobin/presenters/(json.js, text.js, index.js) — net −460 LOC--ndjsonflag for compact single-line JSON output (sorted keys,_-prefixed keys stripped)NO_COLOR/FORCE_COLOR/CI/ pipe detection for automatic ANSI stripping_renderedSvg/_renderedAsciifrom--jsonoutput (bugfix)Test plan
test/unit/presenters/(json, text, present)test/bats/cli-ndjson.batsnpm run test:node22)Summary by CodeRabbit
Release 10.8.0
New Features
Bug Fixes
Documentation
Tests