Conversation
📝 WalkthroughWalkthroughSwaps CI from Node/npm to Bun across workflow jobs, adds a Build Binaries job and artifact upload, introduces scripts to generate bundled figlet fonts and to build cross-platform binaries, updates package.json scripts/devDependencies, extends .gitignore, and calls font initialization at CLI startup. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GH as GitHub Actions
participant Repo as Repository (scripts, src)
participant Bun as Bun tool/runtime
participant FS as Filesystem (dist/bin, src/fonts.ts)
participant Npm as npm registry
GH->>Repo: checkout
GH->>Bun: setup Bun (action/setup-bun)
GH->>Bun: bun install / bun run prebuild
GH->>Repo: run `scripts/generate-fonts.ts`
Repo->>FS: write `src/fonts.ts`
GH->>Bun: bun run build:all (invokes build-binaries)
Bun->>Repo: execute `scripts/build-binaries.ts`
Repo->>Bun: bun build (per-target)
Bun->>FS: emit `dist/bin/*` artifacts
GH->>GH: upload artifacts
GH->>Bun: bun install/build in Publish job
GH->>Npm: npm publish (preserves npm publish step)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
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 |
# Conflicts: # .github/workflows/node.yml # package-lock.json
There was a problem hiding this comment.
Pull request overview
This PR adds Bun support to enable building standalone executables for the w67 CLI tool across multiple platforms (macOS, Linux, Windows). The key changes include:
- Replacing npm with Bun as the package manager and build tool
- Adding scripts to generate font bundles and build cross-platform binaries
- Removing the Node.js shebang from the CLI entry point
- Updating CI/CD workflows to use Bun
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/cli.tsx | Removed Node.js shebang; added font initialization call at module load |
| scripts/generate-fonts.ts | New script to bundle all figlet fonts into a single importable module for compiled binaries |
| scripts/build-binaries.ts | New script to build standalone executables for 7 different platform/architecture combinations |
| package.json | Added Bun-based build scripts; added react-devtools-core v7.0.1 as devDependency |
| package-lock.json | Deleted npm lockfile |
| bun.lock | Added new Bun lockfile |
| .gitignore | Added entries for generated fonts file and compiled binaries |
| .github/workflows/node.yml | Migrated CI workflow from npm to Bun; added binary build job |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @package.json:
- Line 14: Update the package.json "build" script so fonts are generated before
TypeScript compilation: modify the "build" script to run the existing
"build:fonts" step prior to running tsc (or add a "prebuild" script that runs
"build:fonts"); ensure this produces the ./fonts.js file required by src/cli.tsx
so that npm run build and the prepublishOnly step succeed.
🧹 Nitpick comments (5)
scripts/generate-fonts.ts (1)
17-23: Add error handling for missing or empty fonts directory.The script will fail silently or with unclear errors if
node_modules/figlet/importable-fontsdoesn't exist or contains no font files. Consider validating before proceeding.🔎 Suggested validation checks
+import { existsSync, readdirSync, writeFileSync } from "fs"; -import { readdirSync, writeFileSync } from "fs"; import { join, dirname } from "path"; import { fileURLToPath } from "url"; const __filename = fileURLToPath(import.meta.url); const __dirname = dirname(__filename); const fontsDir = join(__dirname, "../node_modules/figlet/importable-fonts"); const outputFile = join(__dirname, "../src/fonts.ts"); +// Validate fonts directory exists +if (!existsSync(fontsDir)) { + console.error(`Error: Fonts directory not found: ${fontsDir}`); + console.error("Please run 'npm install' or 'bun install' first."); + process.exit(1); +} + // Get all .js font files (exclude .d.ts type definitions) const fontFiles = readdirSync(fontsDir) .filter((f) => f.endsWith(".js")) .map((f) => f.replace(/\.js$/, "")); +// Validate we found fonts +if (fontFiles.length === 0) { + console.error(`Error: No font files found in ${fontsDir}`); + process.exit(1); +} + console.log(`Found ${fontFiles.length} fonts`);scripts/build-binaries.ts (2)
25-42: Validate that dist/cli.js exists before building binaries.The script assumes
dist/cli.jsexists but doesn't verify it. If TypeScript compilation hasn't run or failed, the builds will fail with unclear errors.🔎 Suggested validation
import { $ } from "bun"; -import { mkdir } from "fs/promises"; +import { mkdir, access } from "fs/promises"; +import { constants } from "fs"; const targets = [ // ... targets ... ]; console.log("Building binaries for all platforms...\n"); +// Validate dist/cli.js exists +try { + await access("dist/cli.js", constants.F_OK); +} catch { + console.error("Error: dist/cli.js not found. Run 'bun run build' first."); + process.exit(1); +} + await mkdir("dist/bin", { recursive: true });
38-41: Consider more descriptive error messages.The current error output only shows "Failed" without context. Consider logging the actual error or build command details to aid debugging.
🔎 Enhanced error reporting
} catch (error) { - console.error(` ✗ ${out} - Failed`); + console.error(` ✗ ${out} - Failed:`, error instanceof Error ? error.message : error); failed++; }.github/workflows/node.yml (2)
16-19: Consider pinning Bun version and enabling dependency caching.The Bun setup doesn't specify a version, which could lead to inconsistent builds across runs if Bun releases breaking changes. Additionally, enabling caching would speed up CI runs.
🔎 Suggested improvements
- uses: actions/checkout@v4 - - uses: oven-sh/setup-bun@v2 + - uses: oven-sh/setup-bun@v2 + with: + bun-version: latest # or pin to a specific version like '1.0.0' - run: bun installFor caching, you can also add:
- uses: actions/cache@v4 with: path: ~/.bun/install/cache key: ${{ runner.os }}-bun-${{ hashFiles('**/bun.lockb') }} restore-keys: | ${{ runner.os }}-bun-
60-61: Remove redundant build step in publish job.The publish job runs
bun run buildeven though it already depends on thebuildjob. This duplicates work and wastes CI time. Consider either:
- Removing the build step and relying on the dependency, or
- Downloading build artifacts from the build job if they're needed.
Since you're rebuilding anyway, the
buildjob dependency might also be redundant unless you want to ensure basic build validation passes before attempting to publish.🔎 Option 1: Remove redundant build step
- run: bun install - - run: bun run build - run: npm publish --provenance --access publicNote: This assumes the checkout and install are sufficient, or you download build artifacts. You may need to verify what files npm publish expects.
🔎 Option 2: Download build artifacts instead of rebuilding
- run: bun install - - run: bun run build + - uses: actions/download-artifact@v4 + with: + name: build-output + path: dist/ - run: npm publish --provenance --access publicNote: This requires the build job to also upload artifacts.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/node.yml.gitignorepackage.jsonscripts/build-binaries.tsscripts/generate-fonts.tssrc/cli.tsx
🧰 Additional context used
🪛 GitHub Actions: CI
src/cli.tsx
[error] 6-6: TS2307: Cannot find module './fonts.js' or its corresponding type declarations.
🔇 Additional comments (7)
.gitignore (1)
144-152: LGTM! Appropriate ignores for build artifacts.The new ignore patterns correctly exclude generated files (fonts.ts), build outputs (dist/bin/), and compiled binaries (w67, w67.exe) from version control.
scripts/generate-fonts.ts (1)
28-35: LGTM! Clean approach to avoid import naming conflicts.Using numbered variable names (font0, font1, etc.) effectively prevents naming collisions with font names that might be invalid JavaScript identifiers.
package.json (2)
15-17: LGTM! Build scripts correctly chain dependencies.The new build scripts properly sequence operations:
build:fonts→ generates font registrybuild:binary→ fonts → TypeScript → single binarybuild:all→ fonts → TypeScript → multi-platform binaries
51-51: Clarify the purpose of react-devtools-core.This appears to be added as a peer dependency requirement for the
inkpackage (v6.5.1), which declaresreact-devtools-coreas a peer dependency. Please add a comment in package.json or clarify in the PR description that this was added to satisfyink's peer dependency requirement.scripts/build-binaries.ts (1)
11-23: LGTM! Comprehensive platform coverage.The target list includes appropriate coverage across macOS, Linux (glibc and musl), and Windows on multiple architectures.
.github/workflows/node.yml (2)
30-42: Artifact path correctly matches build script output. Thedist/bin/directory specified in the artifact upload aligns with the output path defined inscripts/build-binaries.ts(line 35:outfile dist/bin/${out}).
44-62: Remove concern about downloading binaries—they are for releases, not npm publishing.The binaries job creates standalone executables in
dist/bin/which are uploaded as artifacts for GitHub releases. However, the npm package only includes files in thedistdirectory (per the"files"field in package.json) and relies on the compiled JavaScript, not these binary executables. The"bin"field points todist/cli.js(compiled JavaScript), not a binary artifact.The workflow is correct as-is. The dependency on the
binariesjob is appropriate for ensuring the build completes successfully before publishing, even if the artifacts aren't downloaded.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/cli.tsx:
- Line 10: The call to initializeFonts() runs at module load with no error
handling; wrap the call in a try/catch (or move it into your main() startup
flow) so font initialization failures don't crash the app: locate
initializeFonts() in src/cli.tsx, surround the invocation with try { await or
call initializeFonts(); } catch (err) { console.error("Font initialization
failed:", err); /* fallback behavior or exit gracefully */ } and if you choose
async startup, remove the top-level call and invoke initializeFonts() inside
main() with the same try/catch.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.jsonsrc/cli.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @scripts/build-binaries.ts:
- Around line 15-18: Wrap the build invocation in a try/catch to avoid silent
crashes: for each iteration over targets (the loop using targets, out and the
await $`bun build ... --outfile dist/bin/${out}` invocation) catch any thrown
error, log a clear diagnostic including the target/out and the error
stack/message (e.g., console.error or processLogger.error), and ensure the
process exits with a non-zero status (process.exit(1)) or rethrows after
logging; optionally collect per-target failures and fail fast on the first error
or report all failures after the loop depending on desired behavior.
🧹 Nitpick comments (1)
scripts/build-binaries.ts (1)
15-18: Consider parallelizing builds for faster execution.The script builds binaries sequentially, which may be slow for 8 targets. Consider parallelizing with
Promise.allto reduce total build time.🔎 Proposed refactor for parallel execution
-for (const target of targets) { +await Promise.all(targets.map(async (target) => { const out = `w67-${target.replace("bun-", "")}${target.includes("windows") ? ".exe" : ""}`; try { console.log(`Building ${out}...`); await $`bun build ./dist/cli.js --compile --target=${target} --outfile dist/bin/${out}`; console.log(` ✓ ${out} - Success`); } catch (error) { console.error(` ✗ ${out} - Failed`); console.error(error); process.exit(1); } -} +}));Note: This assumes your build environment can handle concurrent builds.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
package.jsonscripts/build-binaries.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.