[codex] Fix banner contrast across terminals#60
Conversation
📝 WalkthroughWalkthroughThe PR refactors ASCII banner rendering to use configurable ANSI escape codes (blue and terminal-default foreground), exports a new ChangesBanner Rendering and Daemon Integration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/cli/ascii-banner.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc src/cli/commands/daemon.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc test/cli/ascii-banner.test.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cli/ascii-banner.test.ts (1)
29-33: ⚡ Quick winAdd an explicit
NO_COLOR+forceColorprecedence test.Please add a case asserting
renderBanner({ env: { NO_COLOR: "1" }, forceColor: true, stdoutIsTTY: false })stays uncolored, so the intended precedence is locked in by tests.As per coding guidelines, "Add a test when you add a feature or fix a bug".
Also applies to: 41-45
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/cli/ascii-banner.test.ts` around lines 29 - 33, Add a test that asserts renderBanner({ env: { NO_COLOR: "1" }, forceColor: true, stdoutIsTTY: false }) produces an uncolored banner (i.e., does NOT contain color escape sequences like "\x1b[94m"), ensuring NO_COLOR takes precedence over forceColor; add the same precedence assertion for the parallel case around the other test (the one similar to the block at lines 41-45) so both capture-paths verify NO_COLOR wins over forceColor.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/cli/ascii-banner.test.ts`:
- Around line 29-33: Add a test that asserts renderBanner({ env: { NO_COLOR: "1"
}, forceColor: true, stdoutIsTTY: false }) produces an uncolored banner (i.e.,
does NOT contain color escape sequences like "\x1b[94m"), ensuring NO_COLOR
takes precedence over forceColor; add the same precedence assertion for the
parallel case around the other test (the one similar to the block at lines
41-45) so both capture-paths verify NO_COLOR wins over forceColor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d7bdbf71-32ad-4a94-8fba-732e240d7c1c
📒 Files selected for processing (3)
src/cli/ascii-banner.tssrc/cli/commands/daemon.tstest/cli/ascii-banner.test.ts
1 participant
Summary
NO_COLOR.Refs #58.
Refs #59.
Validation
npm run build && npm run build:testnpm run test:cli -- test/cli/daemon.test.ts -t "daemon's own cleanup kills kubo after SIGTERM"npm run test:cli(29 files, 215 passed, 1 skipped)Note
Low Risk
Changes are limited to terminal styling, banner API surface, and daemon startup ordering for safer SIGTERM cleanup; no auth or data-path logic changes.
Overview
Improves CLI banner readability by dropping fixed RGB silver/blue in favor of a standard bright blue accent (
\x1b[94m) and the terminal’s default foreground for the wordmark and rings, so the art works on light and dark themes. Banner rendering is refactored intorenderBannerwith injectable env/TTY/forceColoroptions and stricterFORCE_COLORhandling; the daemon callsprintBanner({ forceColor: true })so Docker/systemd logs still get color whileNO_COLORstays honored.Daemon startup order moves Kubo bring-up and RPC connection to after
asyncExitHookand the emergencyexithandler are registered, closing a window where SIGTERM during early readiness could run before cleanup was wired.Adds
test/cli/ascii-banner.test.tscovering plain output, color codes, non-TTY behavior, forced color, andFORCE_COLORedge cases.Reviewed by Cursor Bugbot for commit 0c1f6b3. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
NO_COLORandFORCE_COLORenvironment variables to manage banner colorsTests