Package npm CLI for Node runtimes#9
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR modernizes the Beeper CLI distribution by replacing a platform-specific binary download mechanism with prebuilt copying, adds WebView cookie-collection support via Bun re-execution, introduces terser-based JavaScript minification, and updates the npm package manifest with dependencies and oclif CLI framework configuration. ChangesCLI Distribution Modernization and WebView Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
🧹 Nitpick comments (1)
packages/npm/package.json (1)
22-37: ⚡ Quick winGitHub branch reference may cause non-reproducible builds.
The
@beeper/desktop-apidependency referencesgithub:beeper/desktop-api-js#next, which tracks a branch rather than a specific commit. This means builds at different times could resolve to different code, making debugging and rollbacks harder.Consider pinning to a specific commit SHA or published version for reproducibility:
- "`@beeper/desktop-api`": "github:beeper/desktop-api-js#next", + "`@beeper/desktop-api`": "github:beeper/desktop-api-js#<commit-sha>",🤖 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 `@packages/npm/package.json` around lines 22 - 37, The dependency entry for "`@beeper/desktop-api`" in package.json currently points to the branch ref "github:beeper/desktop-api-js#next", which can produce non-reproducible builds; update that dependency in package.json to a stable reference by replacing "github:beeper/desktop-api-js#next" with either a published semver version (e.g., "1.2.3") or a full commit SHA reference (e.g., "github:beeper/desktop-api-js#<commit-sha>"), then run your lockfile update (npm install / npm ci / yarn install) to regenerate package-lock.json or yarn.lock so builds are deterministic.
🤖 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.
Inline comments:
In `@packages/cli/src/commands/accounts/add.ts`:
- Around line 90-93: Replace the loose existence check of globalThis.Bun with
explicit checks for Bun and Bun.WebView: if globalThis.Bun is undefined, return
early (same behavior); else if Bun exists but Bun.WebView is falsy, throw a
clear Error stating Bun.WebView is unavailable (use the existing message or
similar). Update the conditional logic around the existing lines that reference
(globalThis as { Bun?: unknown }).Bun and the
process.env.BEEPER_CLI_WEBVIEW_REEXEC branch so callers (and account-login.ts)
get an immediate, specific error when Bun.WebView is missing.
In `@packages/npm/scripts/build.ts`:
- Line 3: Remove the unused `dirname` import from the top-level import
statement: update the import from 'node:path' to only import `join` (remove
`dirname`) since `dirname` is not referenced in this module and the launcher
code uses a string literal instead; locate the import line that currently reads
`import { dirname, join } from 'node:path'` and change it so only `join` is
imported.
---
Nitpick comments:
In `@packages/npm/package.json`:
- Around line 22-37: The dependency entry for "`@beeper/desktop-api`" in
package.json currently points to the branch ref
"github:beeper/desktop-api-js#next", which can produce non-reproducible builds;
update that dependency in package.json to a stable reference by replacing
"github:beeper/desktop-api-js#next" with either a published semver version
(e.g., "1.2.3") or a full commit SHA reference (e.g.,
"github:beeper/desktop-api-js#<commit-sha>"), then run your lockfile update (npm
install / npm ci / yarn install) to regenerate package-lock.json or yarn.lock so
builds are deterministic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9804022-ccac-4bbc-8d09-90dcd7fd672b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
package.jsonpackages/cli/src/commands/accounts/add.tspackages/npm/package.jsonpackages/npm/scripts/build.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (macos-latest / bun 1.3.10)
🧰 Additional context used
🪛 ESLint
packages/cli/src/commands/accounts/add.ts
[error] 111-111: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 111-111: Only use process.exit() in CLI apps. Throw an error instead.
(unicorn/no-process-exit)
packages/npm/scripts/build.ts
[error] 3-3: 'dirname' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (7)
package.json (1)
33-33: LGTM!packages/npm/package.json (1)
4-21: LGTM!Also applies to: 38-121
packages/npm/scripts/build.ts (3)
31-45: LGTM!
47-67: LGTM!
16-29: LGTM!packages/cli/src/commands/accounts/add.ts (2)
2-2: LGTM!Also applies to: 34-34
95-112: LGTM!
| if ((globalThis as { Bun?: unknown }).Bun) return | ||
| if (process.env.BEEPER_CLI_WEBVIEW_REEXEC === '1') { | ||
| throw new Error('Account web views are unavailable because this Bun runtime does not expose Bun.WebView.') | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find usage patterns of Bun.WebView to confirm the correct check
# Search for Bun.WebView usage
rg -nP --type=ts -C3 'Bun\.WebView'
# Search for Bun global checks
rg -nP --type=ts -C3 'globalThis.*Bun'Repository: beeper/cli
Length of output: 4532
Check specifically for Bun.WebView, not just Bun.
The current check on line 90 verifies that the Bun global exists, but the feature requires Bun.WebView specifically. If the code runs under a Bun runtime that exposes the Bun global but lacks Bun.WebView, the function returns early and later code in account-login.ts will throw a generic error when it tries to use WebView. Checking both conditions separately provides immediate, clear error messages for each case.
🔍 Proposed fix
function reexecWithBunForWebViewIfNeeded(): void {
- if ((globalThis as { Bun?: unknown }).Bun) return
+ const hasBunWebView = Boolean((globalThis as { Bun?: { WebView?: unknown } }).Bun?.WebView)
+ const hasBun = Boolean((globalThis as { Bun?: unknown }).Bun)
+
+ if (hasBunWebView) return
+
+ if (hasBun) {
+ throw new Error('Account web views require Bun.WebView, which is not available in this Bun runtime. Provide cookies manually with --cookie.')
+ }
+
if (process.env.BEEPER_CLI_WEBVIEW_REEXEC === '1') {
throw new Error('Account web views are unavailable because this Bun runtime does not expose Bun.WebView.')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ((globalThis as { Bun?: unknown }).Bun) return | |
| if (process.env.BEEPER_CLI_WEBVIEW_REEXEC === '1') { | |
| throw new Error('Account web views are unavailable because this Bun runtime does not expose Bun.WebView.') | |
| } | |
| const hasBunWebView = Boolean((globalThis as { Bun?: { WebView?: unknown } }).Bun?.WebView) | |
| const hasBun = Boolean((globalThis as { Bun?: unknown }).Bun) | |
| if (hasBunWebView) return | |
| if (hasBun) { | |
| throw new Error('Account web views require Bun.WebView, which is not available in this Bun runtime. Provide cookies manually with --cookie.') | |
| } | |
| if (process.env.BEEPER_CLI_WEBVIEW_REEXEC === '1') { | |
| throw new Error('Account web views are unavailable because this Bun runtime does not expose Bun.WebView.') | |
| } |
🤖 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 `@packages/cli/src/commands/accounts/add.ts` around lines 90 - 93, Replace the
loose existence check of globalThis.Bun with explicit checks for Bun and
Bun.WebView: if globalThis.Bun is undefined, return early (same behavior); else
if Bun exists but Bun.WebView is falsy, throw a clear Error stating Bun.WebView
is unavailable (use the existing message or similar). Update the conditional
logic around the existing lines that reference (globalThis as { Bun?: unknown
}).Bun and the process.env.BEEPER_CLI_WEBVIEW_REEXEC branch so callers (and
account-login.ts) get an immediate, specific error when Bun.WebView is missing.
| import { chmod, cp, mkdir, readFile, rm, writeFile } from 'node:fs/promises' | ||
| import { existsSync } from 'node:fs' | ||
| import { join } from 'node:path' | ||
| import { dirname, join } from 'node:path' |
There was a problem hiding this comment.
Remove unused dirname import.
Static analysis correctly identifies that dirname is imported but never used in this file. The launcher function's generated code uses dirname, but that's a string literal, not this import.
Proposed fix
-import { dirname, join } from 'node:path'
+import { join } from 'node:path'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { dirname, join } from 'node:path' | |
| import { join } from 'node:path' |
🧰 Tools
🪛 ESLint
[error] 3-3: 'dirname' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 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 `@packages/npm/scripts/build.ts` at line 3, Remove the unused `dirname` import
from the top-level import statement: update the import from 'node:path' to only
import `join` (remove `dirname`) since `dirname` is not referenced in this
module and the launcher code uses a string literal instead; locate the import
line that currently reads `import { dirname, join } from 'node:path'` and change
it so only `join` is imported.
batuhan
force-pushed
the
codex/npm-runtime-cli
branch
from
506c140 to
cf88821
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/npm/package.json`:
- Line 23: Replace the moving branch ref for the dependency key
"`@beeper/desktop-api`" in package.json so installs are reproducible: change the
value currently using "`#next`" to the immutable commit SHA
"550009b71f922f605d4ca0e0059bbed6627e5fbe" (e.g.
"github:beeper/desktop-api#550009b71f922f605d4ca0e0059bbed6627e5fbe") or to a
published semver release; update the package.json entry for
"`@beeper/desktop-api`" and run a fresh install to verify lockfile changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e910f2f4-89db-4ad5-9b1a-dec4e549e051
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
package.jsonpackages/cli/src/commands/accounts/add.tspackages/npm/package.jsonpackages/npm/scripts/build.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
- GitHub Check: test (macos-latest / bun 1.3.10)
- GitHub Check: test (ubuntu-latest / bun 1.3.10)
🧰 Additional context used
🪛 ESLint
packages/cli/src/commands/accounts/add.ts
[error] 110-110: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 110-110: Only use process.exit() in CLI apps. Throw an error instead.
(unicorn/no-process-exit)
[error] 126-126: Don't use process.exit(); throw an error instead.
(n/no-process-exit)
[error] 126-126: Only use process.exit() in CLI apps. Throw an error instead.
(unicorn/no-process-exit)
[error] 141-141: Expected blank line before this statement.
(@stylistic/padding-line-between-statements)
packages/npm/scripts/build.ts
[error] 3-3: 'dirname' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (5)
packages/cli/src/commands/accounts/add.ts (2)
92-95: Still missing an explicitBun.WebViewcapability check before returning.This still short-circuits on
Bunpresence alone, so environments withBunbut withoutBun.WebViewwon’t fail early with the intended clear message.
2-3: LGTM!Also applies to: 5-5, 36-37, 129-150
packages/npm/scripts/build.ts (2)
3-3: Already reported: remove unuseddirnameimport.
5-68: LGTM!packages/npm/package.json (1)
4-22: LGTM!Also applies to: 24-122
| "typecheck": "bun build scripts/build.ts --target=bun --outdir=/tmp/beeper-cli-npm-check --entry-naming='[name].js'" | ||
| }, | ||
| "dependencies": { | ||
| "@beeper/desktop-api": "github:beeper/desktop-api-js#next", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Declared dependency:"
jq -r '.dependencies["`@beeper/desktop-api`"]' packages/npm/package.json
echo
echo "Current commit behind 'next' branch:"
curl -s https://api.github.com/repos/beeper/desktop-api-js/commits/next | jq -r '.sha, .commit.committer.date'Repository: beeper/cli
Length of output: 209
Pin @beeper/desktop-api to an immutable version.
Line 23 uses a moving branch ref (#next), which makes installs non-reproducible and increases supply-chain risk for published npm consumers. The current commit behind the next branch is 550009b71f922f605d4ca0e0059bbed6627e5fbe. Replace the branch reference with this or a stable commit SHA:
- "`@beeper/desktop-api`": "github:beeper/desktop-api-js#next",
+ "`@beeper/desktop-api`": "github:beeper/desktop-api-js#550009b71f922f605d4ca0e0059bbed6627e5fbe",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "@beeper/desktop-api": "github:beeper/desktop-api-js#next", | |
| "`@beeper/desktop-api`": "github:beeper/desktop-api-js#550009b71f922f605d4ca0e0059bbed6627e5fbe", |
🤖 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 `@packages/npm/package.json` at line 23, Replace the moving branch ref for the
dependency key "`@beeper/desktop-api`" in package.json so installs are
reproducible: change the value currently using "`#next`" to the immutable commit
SHA "550009b71f922f605d4ca0e0059bbed6627e5fbe" (e.g.
"github:beeper/desktop-api#550009b71f922f605d4ca0e0059bbed6627e5fbe") or to a
published semver release; update the package.json entry for
"`@beeper/desktop-api`" and run a fresh install to verify lockfile changes.
|
@indent review |
|
|
Review posted in the banner. Summary of what I found after building the package locally and exercising Functional / latent
Nits
Build + minified launcher both work end-to-end on my sandbox (oclif command discovery, help, version), and the |
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| const entrypoint = process.argv[1] | ||
| if (!entrypoint) throw new Error('Account web views require Bun. Install Bun and retry, or provide cookies manually with --cookie.') | ||
|
|
||
| const binary = findInstalledBeeperBinary(entrypoint) |
There was a problem hiding this comment.
Latent: re-exec into a non-Bun beeper produces a confusing error. findInstalledBeeperBinary only filters by realpath, so any other Node-based beeper earlier on PATH (e.g. a second node_modules/.bin/beeper from a sibling project, or a future Node-only build) is treated as a valid Bun target. We then spawn it with BEEPER_CLI_WEBVIEW_REEXEC=1; the child re-enters this function, hits the process.env.BEEPER_CLI_WEBVIEW_REEXEC === '1' guard at line 93, and throws Account web views are unavailable because this Bun runtime does not expose Bun.WebView. — even though no Bun runtime was ever invoked.
Consider verifying the candidate is actually a Bun-compiled binary before re-execing (e.g. probe it for globalThis.Bun), or rewording the guard error to say "Re-exec did not land in a Bun runtime" so it's not misattributed to a missing WebView module.
| "@oclif/plugin-plugins": "^5.4.67", | ||
| "@oclif/plugin-warn-if-update-available": "^3.1.49", | ||
| "figures": "^6.1.0", | ||
| "ink": "^7.0.3", |
There was a problem hiding this comment.
Maintenance: dependency block is now hand-duplicated from packages/cli/package.json. Every entry here — @oclif/core, @oclif/plugin-*, ink, react, qrcode, ws, @beeper/desktop-api, @beeper/cli-plugin-cloudflare, etc. — must stay in sync with packages/cli/package.json. Nothing currently enforces that. When someone later bumps @oclif/core in the cli but not here, the npm package will publish with mismatched versions (or missing modules) and break at runtime in users' shells.
Consider adding a CI check that diffs the two dependencies blocks (and the oclif plugins/jitPlugins arrays which are also duplicated), or generating this block from packages/cli/package.json at build time.
| "scripts": { | ||
| "build": "bun scripts/build.ts", | ||
| "clean": "rm -rf bin binaries.json README.md LICENSE", | ||
| "clean": "rm -rf bin dist README.md LICENSE", |
There was a problem hiding this comment.
CI gap: test doesn't exercise the published artifact. bun run build only produces the bundle; it never runs node bin/beeper.js against it. With terser doing aggressive mangle + compress + module: true minification per file, a future change to terser options, an oclif version bump, or a new top-level import.meta/await usage in dist/**/*.js could break the published launcher silently.
Consider extending this to something like:
"test": "bun run build && BEEPER_NO_LOGO=1 node bin/beeper.js --version && BEEPER_NO_LOGO=1 node bin/beeper.js --help > /dev/null"so CI smoke-runs the artifact before it ships to npm.
| await cp(join(cliRoot, 'README.md'), join(root, 'README.md')) | ||
| await cp(join(cliRoot, 'LICENSE'), join(root, 'LICENSE')) | ||
| await writeFile(join(root, 'binaries.json'), `${JSON.stringify(binaries, null, 2)}\n`) | ||
| await cp(join(cliRoot, 'dist'), join(root, 'dist'), { recursive: true }) |
There was a problem hiding this comment.
Nit: tarball bloat. This copies the entire cli dist verbatim, which means the published beeper-cli package ships all of tsc's .d.ts and .d.ts.map files — 138 of them, ~900KB — even though packages/npm isn't a library and nothing imports those types. Either filter to *.js/*.json in this cp, or follow up with a sweep that removes **/*.d.ts and **/*.d.ts.map from packages/npm/dist.
1 participant
Summary
accounts add --webviewthroughbunwhen Node is running and report a clear unavailable message when Bun/WebView is unavailableVerification
bun run --filter ./packages/npm testbun run --filter @beeper/cli typecheckbun run --filter @beeper/cli testnode packages/npm/bin/beeper.js --helpnode packages/npm/bin/beeper.js accounts add --helpbun pm pack --dry-runinpackages/npmnode packages/npm/bin/beeper.js accounts add discord --webview --non-interactivereports Bun is required