feat: facet-cli#51
Merged
Merged
Conversation
WalkthroughThe PR migrates the CLI from Bun to Node.js, adds Node SEA binary generation and a new ChangesNode SEA CLI migration
Possibly related PRs
🚥 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)
✨ Simplify code
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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/test.yml (1)
27-30: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNode version drift: CI tests on 20, release/Docker build on 22.
This job still pins
node-version: "20"whilerelease.ymland the Dockerfile now use Node 22. Since the “Build (library + SEA binary)” + smoke-test steps below build a SEA binary that embeds the host Node, CI validates a Node-20 binary while production ships Node 22. Aligning to 22 keeps the smoke test representative of what’s released.Suggested change
with: - node-version: "20" + node-version: "22" cache: "pnpm"🤖 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 @.github/workflows/test.yml around lines 27 - 30, The CI workflow is still pinned to Node 20 while the release and Docker build use Node 22, so update the Setup Node.js step in the test workflow to match the Node version used by the release path. Make the change in the workflow job that runs the Build (library + SEA binary) and smoke-test steps, using the existing actions/setup-node configuration, so the embedded Node version tested in CI matches what ships.cli/src/utils/pdf-multipass.ts (1)
459-521: 🎯 Functional Correctness | 🟠 MajorRestore a fallback for placeholder overlays
cli/src/utils/pdf-multipass.ts:487-513
WhenheaderHasPlaceholders/footerHasPlaceholdersis true butbrowserorhtmlis missing,embeddedstays unset and the overlay is skipped entirely. Keep the old stream-rewrite path here, or make marker-bearing overlays requirebrowser/htmlat the call sites.🤖 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 `@cli/src/utils/pdf-multipass.ts` around lines 459 - 521, The placeholder overlay handling in pdf-multipass currently drops header/footer content when headerHasPlaceholders or footerHasPlaceholders is true but browser or html is unavailable. Update the page loop in pdf-multipass so marker-bearing overlays still render via the existing fallback path instead of leaving embedded unset, or enforce browser/html availability earlier at the call sites that invoke renderElementPdf and embedOverlay.cli/src/bundler/vite-server.ts (1)
67-72: 🩺 Stability & Availability | 🟠 MajorBound dev-server shutdown.
close()waits onexitwith no timeout, so cleanup can hang indefinitely if the child ignoresSIGTERM. Add a timeout and fall back toSIGKILL.🤖 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 `@cli/src/bundler/vite-server.ts` around lines 67 - 72, The close() path in vite-server’s dev-server process cleanup can hang forever because it waits unconditionally on proc’s exit event after proc.kill(). Update the close logic to add a bounded timeout around the await once(proc, 'exit') in the proc shutdown flow, and if the process has not exited by then, force-terminate it with SIGKILL before continuing cleanup. Keep the fix localized to the close() implementation that manages proc.exitCode and proc.signalCode.
🧹 Nitpick comments (2)
cli/scripts/build-sea.cjs (2)
13-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: fail fast if the bundle is missing.
bundle(dist-sea/cli.cjs) is assumed to exist; ifbuild:bundlewas skipped,--experimental-sea-configfails with an opaque error referencingmain. A quickexistsSync(bundle)check with a clear message improves the build-time DX.🤖 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 `@cli/scripts/build-sea.cjs` around lines 13 - 33, The SEA build step in build-sea.cjs assumes the cli.cjs bundle already exists, which leads to an opaque failure when --experimental-sea-config runs without it. Add an existsSync check for the bundle path before writing sea-config.json or calling execFileSync, and throw a clear error message that tells the user the bundle is missing and that build:bundle must be run first; use the bundle, seaConfigPath, and execFileSync flow in build-sea.cjs to place the guard in the right spot.
38-53: 🩺 Stability & Availability | 🔵 TrivialWindows release signing is optional.
facet-windows-x64.exeis built in the release workflow, but the SEA injection step doesn’t re-sign it. That won’t prevent the binary from running; add asigntool/re-sign step only if signed Windows artifacts are required.🤖 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 `@cli/scripts/build-sea.cjs` around lines 38 - 53, The SEA injection in build-sea.cjs only re-signs the binary on darwin, so Windows release artifacts built by the same flow remain unsigned after postject injection. Update the build-sea.cjs flow around the execFileSync injection and the existing macOS codesign block to either explicitly add a Windows signing step using the release signing toolchain or leave the behavior unchanged if signed Windows binaries are not required. Keep the platform-specific logic in build-sea.cjs clear by branching on process.platform near the postject and codesign handling.
🤖 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 @.github/workflows/release.yml:
- Line 243: The release workflow step currently interpolates
needs.version-bump.outputs.new_version directly into the run command, which
should be moved to an env binding instead. Update the job step in the release
workflow so the version is passed via an environment variable and then consumed
by the pack-npm-cli.cjs invocation, rather than using shell template expansion.
Use the existing version-bump output and the pack-npm-cli.cjs step as the key
symbols to locate the change.
In `@cli/src/builders/facet-directory.ts`:
- Around line 982-989: The local build invocation in facet-directory’s spawnSync
call can fail with ENOBUFS because the default stdout/stderr buffer is too small
for chatty pnpm scripts. Update the spawnSync options in the local build path to
set a larger maxBuffer alongside cwd and timeout, and keep the existing
stdout/stderr logging behavior in place so build:components and build:css output
can be captured safely.
In `@cli/src/bundler/vite-builder.ts`:
- Around line 126-129: The spawned SSR loader process in runLoaderOnce currently
leaves stdout as a pipe while only stderr is consumed, which can cause the child
to block if it writes enough output. Update the spawn call for child in
vite-builder.ts to either ignore stdout or actively drain/read it, and keep the
fix localized around runLoaderOnce and the child process setup so the loader
cannot hang the build.
In `@cli/src/cli.ts`:
- Line 348: The floating run() call in cli.ts should handle promise rejections
instead of allowing unhandled rejections from the dynamic loaders or
runSsrLoader/runDevLoader. Update the run() invocation to attach an error
handler or await it from an async entrypoint so failures are caught, logged with
a readable message, and translated into a controlled non-zero process exit. Use
the run() symbol and its loader call sites as the place to add the top-level
rejection handling.
In `@cli/src/commands/doctor.ts`:
- Around line 96-101: The preflight flow in doctor.ts is skipping required ids
when run(id) returns undefined, so unknown required checks are treated as
passing. Update the loop in the preflight logic to treat an unresolved required
id as a failure instead of ignoring it, and make sure CHECK_REGISTRY coverage is
enforced for all ids passed from cli.ts or any other caller. Use the run and
requiredIds handling in doctor.ts to locate the fix.
In `@cli/src/loaders/ssr.ts`:
- Around line 87-91: Remove the NODE_PATH and Module._initPaths() mutation from
the SSR loader because facetRequire already handles resolving Vite, React, and
the externalized bundle relative to .facet. Update the setup in ssr.ts to rely
on facetRequire’s existing resolution behavior and delete the
facetNodeModules/originalNodePath logic so the loader no longer depends on the
private Node API.
In `@cli/src/server/archive.ts`:
- Line 5: The archive handling in archive.ts still depends on shelling out
through the imported shell helper, which breaks on Windows. Update the archive
extraction flow in the relevant archive extraction function to use Node-native
directory creation and a cross-platform tar extraction approach, or gate the
feature off on Windows explicitly. Keep the fix localized around the existing
archive upload/extraction logic so the behavior is portable across shipped
binaries.
In `@cli/src/server/preview.ts`:
- Line 101: Bind the preview server explicitly to loopback instead of all
interfaces: update the httpServer.listen call in preview startup so it uses
127.0.0.1 by default unless there is an explicit host configuration. Keep the
existing config.port behavior, and make the binding consistent with the
localhost preview logging and any optional API-key auth behavior in preview.ts.
- Around line 88-95: The request error handling in
createHttpServer/handleRequest only wraps the promise chain, so
nodeToWebRequest(req) can still throw before catch runs. Move the adapter call
into the same request-level try/catch or convert it into a guarded promise flow
so malformed request metadata is handled by the existing logger.error and 500
JSON response path instead of crashing the server.
In `@cli/src/utils/data-loader.ts`:
- Line 4: The data-loader shell command currently invokes bare tsx via the shell
helper, so it depends on tsx being available on PATH. Update the logic in
data-loader’s loader execution path to use the resolved tsx package executable
explicitly (or otherwise ensure the dependency is invoked without relying on
PATH), and keep the behavior in sync with the existing shell-based execution in
this module.
In `@cli/src/utils/remote-resolver.ts`:
- Line 6: The remote clone flow in remote-resolver.ts is leaking credentials
because authedUrl is embedded in a shell-rendered git clone command via $ from
shell.js. Update the clone logic in the relevant resolver functions to avoid
putting GIT_TOKEN into the command string; use a credential helper/askpass-based
auth flow or otherwise pass credentials outside the rendered shell command, and
ensure any propagated shell errors are redacted so the token cannot appear in
logs or process listings.
In `@cli/src/utils/shell.ts`:
- Around line 73-77: ShellPromise.text() does not match its JSDoc because it
returns stdout without trimming; update the implementation in text() or revise
the docstring so they agree. Locate the fix in ShellPromise.text() and either
apply .trim() to the result.stdout string if trimmed output is intended, or
change the comment to describe the actual behavior.
In `@cli/src/utils/tailwind.ts`:
- Line 9: The verbose path in the Tailwind helper is still using the shell
wrapper’s captured output, so diagnostics are hidden just like quiet mode.
Update the Tailwind command runner in tailwind.ts so the opts.verbose branch
either writes the captured stdout/stderr buffers after execution or uses a
shell-helper mode that inherits stdio instead of capturing it. Use the existing
shell helper imported as $ and the verbose branch around the Tailwind invocation
as the main location to fix.
---
Outside diff comments:
In @.github/workflows/test.yml:
- Around line 27-30: The CI workflow is still pinned to Node 20 while the
release and Docker build use Node 22, so update the Setup Node.js step in the
test workflow to match the Node version used by the release path. Make the
change in the workflow job that runs the Build (library + SEA binary) and
smoke-test steps, using the existing actions/setup-node configuration, so the
embedded Node version tested in CI matches what ships.
In `@cli/src/bundler/vite-server.ts`:
- Around line 67-72: The close() path in vite-server’s dev-server process
cleanup can hang forever because it waits unconditionally on proc’s exit event
after proc.kill(). Update the close logic to add a bounded timeout around the
await once(proc, 'exit') in the proc shutdown flow, and if the process has not
exited by then, force-terminate it with SIGKILL before continuing cleanup. Keep
the fix localized to the close() implementation that manages proc.exitCode and
proc.signalCode.
In `@cli/src/utils/pdf-multipass.ts`:
- Around line 459-521: The placeholder overlay handling in pdf-multipass
currently drops header/footer content when headerHasPlaceholders or
footerHasPlaceholders is true but browser or html is unavailable. Update the
page loop in pdf-multipass so marker-bearing overlays still render via the
existing fallback path instead of leaving embedded unset, or enforce
browser/html availability earlier at the call sites that invoke renderElementPdf
and embedOverlay.
---
Nitpick comments:
In `@cli/scripts/build-sea.cjs`:
- Around line 13-33: The SEA build step in build-sea.cjs assumes the cli.cjs
bundle already exists, which leads to an opaque failure when
--experimental-sea-config runs without it. Add an existsSync check for the
bundle path before writing sea-config.json or calling execFileSync, and throw a
clear error message that tells the user the bundle is missing and that
build:bundle must be run first; use the bundle, seaConfigPath, and execFileSync
flow in build-sea.cjs to place the guard in the right spot.
- Around line 38-53: The SEA injection in build-sea.cjs only re-signs the binary
on darwin, so Windows release artifacts built by the same flow remain unsigned
after postject injection. Update the build-sea.cjs flow around the execFileSync
injection and the existing macOS codesign block to either explicitly add a
Windows signing step using the release signing toolchain or leave the behavior
unchanged if signed Windows binaries are not required. Keep the
platform-specific logic in build-sea.cjs clear by branching on process.platform
near the postject and codesign handling.
🪄 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: 443967b6-49cc-4791-8d06-db70979037da
⛔ Files ignored due to path filters (1)
cli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (51)
.github/workflows/release.yml.github/workflows/test.ymlDockerfileREADME.mdTaskfile.ymlcli/.gitignorecli/npm/facet-cli/README.mdcli/npm/facet-cli/package.jsoncli/package.jsoncli/scripts/build-sea.cjscli/scripts/gen-version.cjscli/scripts/pack-npm-cli.cjscli/scripts/pack-npm-cli.test.tscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/renderer.tscli/src/bundler/vite-builder.test.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/cli.tscli/src/commands/doctor.test.tscli/src/commands/doctor.tscli/src/loaders/dev.tscli/src/loaders/ssr.tscli/src/server/archive.tscli/src/server/preview.tscli/src/server/routes.tscli/src/types.tscli/src/utils/assets.test.tscli/src/utils/assets.tscli/src/utils/css-scoper.tscli/src/utils/data-loader.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/pdf-multipass.tscli/src/utils/pdf-security.tscli/src/utils/remote-resolver.test.tscli/src/utils/remote-resolver.tscli/src/utils/self-exec.tscli/src/utils/shell.tscli/src/utils/tailwind.test.tscli/src/utils/tailwind.tscli/test/jsx-parser.test.tscli/test/lint-rules.test.tscli/test/resolve-output.test.tscli/tsconfig.jsoncli/tsdown.config.tscli/tsdown.sea.config.tscli/vite-dev-loader.tscli/vitest.config.tsvite.lib.config.ts
💤 Files with no reviewable changes (1)
- cli/vite-dev-loader.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/server/archive.ts (1)
24-38: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftCap extracted archive size, not just uploaded bytes.
Line 24 only limits the compressed upload size, but Line 38 expands the archive without a cumulative output limit. A small tar.gz bomb can fill disk or tie up the preview server. Use a streaming extractor/preflight that enforces uncompressed byte and entry limits before writing files.
🤖 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 `@cli/src/server/archive.ts` around lines 24 - 38, The archive handling in archive() only checks the compressed upload size and then blindly expands it with tar, so add protection for the uncompressed content as well. Update the extraction flow after writeFile/extractDir to use a streaming or preflight validation step that enforces maximum total extracted bytes and entry/file-count limits before any files are written. Keep the existing ARCHIVE_ERROR path in RenderError, and adjust the tar extraction logic in archive.ts so oversized archives are rejected before disk fill or excessive expansion can occur.
🤖 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 `@cli/src/utils/pdf-multipass.ts`:
- Around line 469-475: The stream rewrite in pdf-multipass is still replacing
the stream body even when the `/Length` value was not actually updated, which
can corrupt PDFs. Update the logic around the dictionary parsing in the stream
rebuild path so `result = Buffer.concat(...)` only runs when a direct `/Length`
match was found and replaced, or handle indirect `/Length` references before
rebuilding. Use the existing `dictStart`, `beforeDict`, `dict`, `afterStream`,
and stream rewrite block to locate the fix.
---
Outside diff comments:
In `@cli/src/server/archive.ts`:
- Around line 24-38: The archive handling in archive() only checks the
compressed upload size and then blindly expands it with tar, so add protection
for the uncompressed content as well. Update the extraction flow after
writeFile/extractDir to use a streaming or preflight validation step that
enforces maximum total extracted bytes and entry/file-count limits before any
files are written. Keep the existing ARCHIVE_ERROR path in RenderError, and
adjust the tar extraction logic in archive.ts so oversized archives are rejected
before disk fill or excessive expansion can occur.
🪄 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: cde72421-759f-4642-b6d7-ce7aa6291ca8
📒 Files selected for processing (16)
.github/workflows/release.yml.github/workflows/test.ymlcli/scripts/build-sea.cjscli/src/builders/facet-directory.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/cli.tscli/src/commands/doctor.tscli/src/loaders/ssr.tscli/src/server/archive.tscli/src/server/preview.tscli/src/utils/data-loader.tscli/src/utils/pdf-multipass.tscli/src/utils/remote-resolver.tscli/src/utils/shell.tscli/src/utils/tailwind.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- .github/workflows/test.yml
- cli/scripts/build-sea.cjs
- cli/src/utils/shell.ts
- cli/src/cli.ts
- cli/src/server/preview.ts
- cli/src/commands/doctor.ts
- .github/workflows/release.yml
- cli/src/bundler/vite-server.ts
- cli/src/builders/facet-directory.ts
- cli/src/loaders/ssr.ts
- cli/src/bundler/vite-builder.ts
Comment on lines
+469
to
+475
| const dictStart = result.lastIndexOf('<<', streamPos); | ||
| if (dictStart !== -1) { | ||
| const beforeDict = result.subarray(0, dictStart); | ||
| const dict = result.subarray(dictStart, dataStart).toString('latin1') | ||
| .replace(/\/Length\s+\d+/, `/Length ${deflated.length}`); | ||
| const afterStream = result.subarray(dataEnd); | ||
| result = Buffer.concat([beforeDict, Buffer.from(dict, 'latin1'), deflated, afterStream]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Avoid rewriting streams when /Length was not updated.
If the stream uses an indirect or missing /Length, the regex leaves the dictionary unchanged while Line 475 still writes a newly deflated stream. That can produce a malformed PDF. Guard on a matched direct length, or implement indirect length updates before rebuilding the stream.
🤖 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 `@cli/src/utils/pdf-multipass.ts` around lines 469 - 475, The stream rewrite in
pdf-multipass is still replacing the stream body even when the `/Length` value
was not actually updated, which can corrupt PDFs. Update the logic around the
dictionary parsing in the stream rebuild path so `result = Buffer.concat(...)`
only runs when a direct `/Length` match was found and replaced, or handle
indirect `/Length` references before rebuilding. Use the existing `dictStart`,
`beforeDict`, `dict`, `afterStream`, and stream rewrite block to locate the fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
@flanksource/facet-clifor npm-based CLI installs and published per-platform standalone binaries.html/pdfand enhancedfacet doctorreporting.