refactor(cli): migrate off Bun runtime to Node#44
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a diagram component library ( ChangesDiagram component library
Live rendering pipeline and local facet override
CLI enhancements: doctor, Tailwind utility, server config, and package manager
Housekeeping: puppeteer-core, ESM migration, CI, and component styling
Sequence DiagramsequenceDiagram
participant CLI as facet html --live
participant generateHTML
participant startViteServer
participant FacetDirectory
participant vite_dev_loader as vite-dev-loader.ts (subprocess)
participant snapshotHTML
participant Puppeteer
CLI->>generateHTML: options.live=true
generateHTML->>startViteServer: templatePath, data
startViteServer->>FacetDirectory: scaffold + generateClientScaffold + installWithRetry
startViteServer->>vite_dev_loader: bun run --facet-root=.facet/
vite_dev_loader-->>startViteServer: stdout: FACET_DEV_URL=http://127.0.0.1:PORT/
startViteServer-->>generateHTML: { url, close() }
generateHTML->>snapshotHTML: url
snapshotHTML->>Puppeteer: launchBrowser, navigate(url)
Puppeteer-->>snapshotHTML: page loaded
snapshotHTML->>Puppeteer: wait data-facet-ready=true on Diagram
snapshotHTML->>Puppeteer: page.content()
snapshotHTML-->>generateHTML: cleaned HTML (dev scripts stripped)
generateHTML->>generateHTML: write outputName.html
generateHTML->>startViteServer: close()
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 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/components/Badge.tsx (1)
432-434:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNullish check regression drops valid
label={0}in non-label variants.Line 433 still uses a truthy guard (
!label) even thoughlabelnow supportsReact.ReactNode. This hides valid values like0(and can misalign label/value rendering). Use a nullish check instead.Proposed fix
- if (!label && !Icon) return null; + if (label == null && !Icon) return null;🤖 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 `@src/components/Badge.tsx` around lines 432 - 434, The renderLabel function currently uses a truthy guard (!label) which hides valid values like 0; update the conditional in renderLabel to use a nullish check (label == null) so that falsy-but-valid nodes (e.g., 0 or empty string) are rendered, keeping the Icon behavior unchanged when both are absent.src/styles.css (1)
415-421:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid forcing left alignment for all
.table-headercolumns.
Line [420](text-align: left) overrides per-column alignment utilities due to higher specificity, so headers likesrc/components/LogoGrid.tsxLines [121-124]won't stay centered. Move alignment control to component classes (or make this rule non-opinionated).Proposed fix
.table-header th { `@apply` font-semibold text-zinc-800; font-size: calc(1em + 1pt); padding: 2mm; - text-align: left; }🤖 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 `@src/styles.css` around lines 415 - 421, The CSS rule ".table-header th" is forcing left alignment via "text-align: left", which overrides per-column alignment utilities (e.g., header cells in LogoGrid.tsx); remove or neutralize that property so alignment is controlled by component-level classes—either delete the "text-align: left" declaration or replace it with a non-opinionated value like "text-align: inherit" on the ".table-header th" selector so per-column utilities can take effect.cli/src/bundler/vite-builder.ts (1)
61-65:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop composing the loader command as raw shell text.
Line 142 builds a raw
/bin/sh -ccommand fromsandboxsettings andtsxpath tokens. This is vulnerable to shell metacharacter injection and also breaks valid paths containing spaces. Build argv arrays and execute without a shell.As per coding guidelines, injection risks should be fixed at the root cause (unsafe command construction).
🛠️ Safer argv-based direction
-async function runLoaderOnce(args: LoaderArgs): Promise<{ stderr?: Buffer | string; }> { - const prefix = srtPrefix(args.sandbox); - const tsx = resolveTsxBin(args.facetRoot).join(' '); - return await $`${{ raw: prefix }}${{ raw: tsx }} ${args.loaderPath} --facet-root=${args.facetRoot} --data-file=${args.dataFilePath} --output-file=${args.resultFilePath}`.quiet(); -} +async function runLoaderOnce(args: LoaderArgs): Promise<{ stderr?: Buffer | string; }> { + const tsxArgv = resolveTsxBin(args.facetRoot); + const baseArgv = [ + ...tsxArgv, + args.loaderPath, + `--facet-root=${args.facetRoot}`, + `--data-file=${args.dataFilePath}`, + `--output-file=${args.resultFilePath}`, + ]; + const argv = args.sandbox + ? ['srt', '--settings', typeof args.sandbox === 'string' ? args.sandbox : '/etc/facet/srt-settings.json', ...baseArgv] + : baseArgv; + // Execute argv without shell expansion (spawn/execFile wrapper). + return await runCommandArgv(argv).quiet(); +}Also applies to: 140-143
🤖 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-builder.ts` around lines 61 - 65, The code currently composes a shell command string (using srtPrefix and then joining with tsx path tokens) which is vulnerable to shell injection and breaks on paths with spaces; instead construct an argv array and run the binary without a shell: change srtPrefix to return the settings token (or undefined) rather than a shell fragment and in the caller (the code that builds and runs the loader command around the tsx path tokens) assemble an argv like ['srt', '--settings', settings, ...tsxArgs] or ['tsx', tsxPath, ...] as appropriate, then invoke the process using a shell-less API (child_process.spawn or execFile / execa with shell: false) so each argument is passed as a distinct argv element and not interpolated into /bin/sh -c. Ensure you preserve handling of sandbox being boolean or string and include settings only when provided.cli/src/generators/html.ts (1)
65-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
clearCachebehavior no longer matches its documented intent.Line 65-72 now clears only
.facet/, so the “node_modules cache” part of the--clear-cachedescription is stale unless that cleanup happens elsewhere.🤖 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/generators/html.ts` around lines 65 - 72, The clearCache branch currently only removes the .facet directory, which no longer matches the flag’s documented intent to also clear the node_modules cache; update the options.clearCache handling (the block that computes root using consumerRoot || resolve(dirname(options.template))) to also detect and remove the node_modules cache under the same root (e.g., compute nodeModulesDir = join(root, 'node_modules') and if existsSync(nodeModulesDir) then rmSync(nodeModulesDir, { recursive: true, force: true }) and log the removal), or if you prefer to change behavior instead, update the CLI docs/help to remove the “node_modules cache” claim — adjust the code around facetDir and the logger calls accordingly.Taskfile.yml (1)
69-74:⚠️ Potential issue | 🟠 MajorFix
test:unitto use Vitest instead of Bun
Taskfile.yml’stest:unittask still runsbun test(and the description says “with bun”), butcli/package.jsondefines the unit test command asvitest run("test": "vitest run"). Replace thecmds: - bun testentry with the appropriate Vitest invocation (e.g.,pnpm testfromdir: cli) and update thedescaccordingly.🤖 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 `@Taskfile.yml` around lines 69 - 74, The Taskfile.yml task currently describes and runs Bun but the CLI package uses Vitest; update the task's desc from "Run cli unit tests with bun" to "Run cli unit tests with Vitest" and replace the cmds entry that currently runs "bun test" with the proper Vitest invocation—e.g., run "pnpm test" (keeping the existing dir: cli and deps: gen-version) so the task executes the CLI package.json "test": "vitest run" command.cli/vite-ssr-loader.ts (1)
109-116:⚠️ Potential issue | 🟠 MajorUse
path.delimiterforNODE_PATHincli/vite-ssr-loader.ts.
Line 112 hardcodes:when buildingprocess.env.NODE_PATH; on WindowsNODE_PATHentries are separated with;(and:can corrupt drive-letter paths), so module resolution can fail. Replace the concatenation to usepath.delimiterinstead (e.g.,${facetNodeModules}${delimiter}${originalNodePath}).🤖 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/vite-ssr-loader.ts` around lines 109 - 116, The NODE_PATH concatenation in vite-ssr-loader uses a hardcoded ':' which breaks on Windows; update the logic that builds process.env.NODE_PATH (using facetNodeModules and originalNodePath) to join entries with path.delimiter instead of ':' and ensure path is imported/available, then call require('module').Module._initPaths() as before to reload resolution; adjust the ternary that sets process.env.NODE_PATH to use `${facetNodeModules}${path.delimiter}${originalNodePath}` when originalNodePath is non-empty.
🧹 Nitpick comments (2)
.github/workflows/test.yml (1)
36-40: ⚡ Quick winVerify whether Bun is still needed in CI after the Node migration.
Line 39 still provisions Bun. If no remaining unit-test/build path calls Bun, remove it to reduce CI setup time and prevent hidden runtime coupling.
🤖 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 36 - 40, The workflow still provisions Bun in the "Setup tools" step (uses: flanksource/deps) by listing bun@v1.3.10 in the tools input; check all CI/workflow steps and repo scripts (package.json scripts, task files, any workflow jobs) for references to bun or bun-specific commands, and if none remain after the Node migration remove bun@v1.3.10 from the tools list (leaving task@v3.39.2) in the Setup tools step to shorten setup time and eliminate hidden coupling, then run the CI locally or trigger the workflow to verify no job fails due to the removal.cli/test/shell.test.ts (1)
15-19: ⚡ Quick winAdd regression coverage for array interpolation and signal termination
The suite validates scalar interpolation, but it misses the two high-risk behaviors used in production: array interpolation and signal-terminated process handling. Adding those tests will lock in the expected contract of
$.Also applies to: 41-51
🤖 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/test/shell.test.ts` around lines 15 - 19, Add two regression tests alongside the existing "shell-escapes interpolated values" test in cli/test/shell.test.ts: one that verifies array interpolation is escaped correctly by invoking the same $ template (e.g., using an array like ['a','b;echo pwned'] interpolated into $`printf '%s\n' ${arr}`) and asserting combined stdout matches expected values, and another that verifies signal-terminated child handling by spawning a process via the $ template, sending it a signal (SIGTERM/SIGINT) and asserting the returned result indicates termination by signal (or the promise rejects with the expected signal info); reuse the existing test harness/quiet() invocation and assertion style so behavior is locked in for both scalar/array interpolation and signal termination cases.
🤖 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:
- Around line 66-69: The checkout step using actions/checkout@v4 currently sets
only ref: ${{ needs.version-bump.outputs.new_tag }}; update that checkout
configuration (the step with name "Checkout code" and uses: actions/checkout@v4)
to include persist-credentials: false so the publish job does not leave Git
credentials in local git config.
- Around line 27-30: The workflow uses mutable action tags (e.g. "uses:
pnpm/action-setup@v4", "uses: actions/checkout@v4", "uses:
actions/setup-node@v4"); replace each "`@v4`" tag with the corresponding pinned
commit SHA from the action's GitHub repository (use the commit SHA for the
release you want) so the workflow references an exact immutable version, and
update all occurrences of those three actions in the file.
In @.github/workflows/test.yml:
- Around line 22-29: The workflow uses mutable tags pnpm/action-setup@v4 and
actions/setup-node@v4 which should be pinned to immutable commit SHAs; update
the uses lines to reference the exact commit SHAs for pnpm/action-setup and
actions/setup-node (e.g., pnpm/action-setup@<commit-sha> and
actions/setup-node@<commit-sha>) by locating the repositories' latest stable
commit for the desired v4 behavior, replace the `@v4` tags with those SHAs, and
verify the workflow still sets the same version inputs (version: 8.13.1 for pnpm
and the Node.js version input) before committing.
In `@cli/README.md`:
- Around line 23-24: Update the global install commands in cli/README.md to use
the actual published package name; replace occurrences of "`@flanksource/facet`"
with "`@flanksource/facet-cli`" in the lines showing pnpm/npm install commands so
they match cli/package.json's "name": "`@flanksource/facet-cli`" and the release
workflow/notes that install the CLI package.
In `@cli/src/builders/facet-directory.ts`:
- Around line 833-835: The generated .npmrc is written with default permissions;
update the write operations (the writeFileSync calls that use npmrcPath and
npmrcContent) to write with restrictive permissions (mode 0o600) and also ensure
existing files that are overwritten have their mode set to 0o600 (e.g., via
fs.chmodSync) so inherited auth tokens are not world-readable; apply the same
change to the other writeFileSync block referenced around the 840-842 region and
reference the npmrcPath/writeFileSync calls when making the edit.
- Around line 508-509: The current code builds dataScript using
JSON.stringify(data) and writes it via writeFileSync to join(this.facetRoot,
'facet-data.js'), which allows script-breakout sequences like </script>;
instead, escape the serialized JSON before embedding: call JSON.stringify(data)
into a variable, then replace unsafe sequences (at minimum </script>, <!--, -->
and the line separator characters U+2028 and U+2029) with safe escaped
equivalents (e.g., replace '</script>' with '<\\/script>' and replace
U+2028/U+2029 with '\\u2028'/'\\u2029'), then build dataScript from the escaped
string and write it using the same writeFileSync to avoid raw injection in
facet-data.js.
- Around line 955-958: The synchronous pnpm invocation using spawnSync in
facet-directory.ts (the call that assigns to result) lacks a timeout and can
hang; update the spawnSync invocation to pass a timeout option (e.g., timeout:
<ms>) and handle the case where result.error or result.status is null/undefined
or where result.signal indicates a timeout by logging/throwing a clear error and
exiting the command. Ensure you reference the spawnSync call that produces the
const result and add logic after that call to detect result.error or a timeout
and fail fast with a helpful message.
In `@cli/src/commands/doctor.ts`:
- Around line 521-532: The checkTsx function currently uses
probeOptionalBin('tsx', ...) which only checks PATH and yields false warnings;
update checkTsx to probe the same resolver the runtime uses instead (e.g., use
the project's runtime resolver or bundled tsx lookup used elsewhere—replace
probeOptionalBin call with the resolver function used by the runtime, such as
resolve/bundled tsx path or getBundledTsxPath), and if that resolver returns a
path, report status 'pass' with its version output; if not found, downgrade the
result to non-failing (change status to 'info' or remove the warn) and keep the
hint only if truly required. Ensure you modify references inside checkTsx and
stop relying solely on probeOptionalBin so the doctor reflects the migrated
bundled/resolved tsx behavior.
In `@cli/src/generators/html.ts`:
- Around line 88-103: The live mode branch (when options.live is true) currently
takes the server from startViteServer, snapshots HTML via snapshotHTML and
writes it out without applying the CSS scoping flow, so --css-scope is ignored;
either apply the same CSS scoping routine used in the SSR path to the captured
html before writing (invoke the existing scoping function used elsewhere) or
explicitly guard and error when options.cssScope is set in conjunction with
options.live (e.g., throw a user-facing error and avoid writing output). Locate
the live branch around options.live, startViteServer, snapshotHTML, outputPath
and writeFile and implement one of those two fixes so scoping behavior matches
the non-live path.
In `@cli/src/server/preview.ts`:
- Around line 156-158: The stream-forwarding loop in writeWebResponse currently
writes each chunk from response.body directly into the Node ServerResponse
without honoring backpressure; modify the loop that iterates over response.body
(the for await (const chunk of response.body ...) block) to check the boolean
result of res.write(chunk) and, if it returns false, await a Promise that
resolves on res.once('drain') before continuing, and ensure res.end() is still
called after the loop completes and any pending backpressure awaits are
resolved; this prevents unbounded buffering when clients are slow.
In `@cli/src/utils/shell.ts`:
- Around line 34-47: The buildCommand currently stringifies interpolated arrays
into one comma-separated token which breaks callers that expect multiple shell
arguments; update buildCommand (and reuse escape/isRaw) so when a value is an
array (Array.isArray(value)) you map each element to either element.raw if
isRaw(element) is true or escape(element) otherwise, then join those mapped
pieces with spaces and append into cmd (instead of treating the whole array as a
single value); leave non-array handling unchanged so single values still use
isRaw(value) ? value.raw : escape(value).
- Around line 90-104: The close handler in cli/src/utils/shell.ts currently
coerces a null code to 0 and treats signal-terminated children as success;
update the child.on('close', ...) callback to accept both (code, signal), treat
any non-null signal as a failure (rejecting with a ShellError that includes
stdout, stderr, signal and a null/undefined exitCode), and only treat numeric
code === 0 as success; modify the logic around the existing Error/ShellError
construction (referencing this.command and ShellError) to include signal details
when rejecting.
In `@examples/kitchen-sink/HeaderSolid.tsx`:
- Around line 54-55: Update the example page title strings that incorrectly
state "10mm" to reflect the configured default header height of "14mm"; find the
JSX elements in HeaderSolid.tsx where the title prop is set to "Page 2 — 10mm
compact chrome" (and the similar title strings on the other occurrences) and
change those strings to "Page 2 — 14mm compact chrome" (and the corresponding
changes for the other two title props so all titles match the 14mm default
header).
In `@README.md`:
- Around line 21-22: Update the global install examples to use the new CLI
package name by replacing occurrences of the string "`@flanksource/facet`" with
"`@flanksource/facet-cli`" in the README examples (the two global install lines
using "pnpm add -g" and "npm install -g"); ensure both commands show the new
package name so users install the CLI package rather than the old library
package.
In `@src/components/diagram/Arrow.tsx`:
- Around line 15-18: The bidirectional arrow preset fails to render tails
because showTail is not included in VariantProps and showTail: true is not set
on the preset; update the VariantProps type to include 'showTail' alongside the
other picked props (so TypeScript will accept it) and add showTail: true to the
bidirectional preset object (alongside its tailShape: 'circle') in Arrow.tsx to
enable tail endpoint rendering.
In `@src/components/diagram/content.tsx`:
- Around line 27-29: The JSX in items.map uses key={item} which can collide for
duplicate labels; update the mapping in the component that renders NodePill so
each element uses a stable unique key (for example combine the item value with
its index or an explicit id) by changing the key expression in the items.map
callback (the NodePill render) to something like `${item}-${index}` or use an
item.id property if available to ensure unique React keys and stable
reconciliation.
In `@src/components/ListTable.tsx`:
- Around line 127-138: splitColorClasses currently drops any non-color tokens so
tagMapping consumers lose layout/typography/shape classes; update
splitColorClasses (and how its result is used in the badge rendering around the
tagMapping code at lines that build badges) to preserve passthrough classes by
collecting all tokens that are not text-*/bg-*/border-* into a passthrough field
(e.g. passthrough?: string[] or passthrough?: string) and return it alongside
textColor/color/borderColor, then update the badge rendering logic (the
tagMapping usage) to append those passthrough classes back into the badge
className so existing layout/shape/typography classes are not dropped.
In `@src/styles.css`:
- Line 4: Replace the `@import` url(...) statement in styles.css with the string
import notation to satisfy stylelint; locate the `@import` statement (the Google
Fonts import at the top of styles.css) and change its syntax from
url("https://fonts.googleapis.com/...") to use a quoted string import (i.e.,
`@import` "https://fonts.googleapis.com/..." ) so the linter no longer flags an
import-notation violation.
---
Outside diff comments:
In `@cli/src/bundler/vite-builder.ts`:
- Around line 61-65: The code currently composes a shell command string (using
srtPrefix and then joining with tsx path tokens) which is vulnerable to shell
injection and breaks on paths with spaces; instead construct an argv array and
run the binary without a shell: change srtPrefix to return the settings token
(or undefined) rather than a shell fragment and in the caller (the code that
builds and runs the loader command around the tsx path tokens) assemble an argv
like ['srt', '--settings', settings, ...tsxArgs] or ['tsx', tsxPath, ...] as
appropriate, then invoke the process using a shell-less API (child_process.spawn
or execFile / execa with shell: false) so each argument is passed as a distinct
argv element and not interpolated into /bin/sh -c. Ensure you preserve handling
of sandbox being boolean or string and include settings only when provided.
In `@cli/src/generators/html.ts`:
- Around line 65-72: The clearCache branch currently only removes the .facet
directory, which no longer matches the flag’s documented intent to also clear
the node_modules cache; update the options.clearCache handling (the block that
computes root using consumerRoot || resolve(dirname(options.template))) to also
detect and remove the node_modules cache under the same root (e.g., compute
nodeModulesDir = join(root, 'node_modules') and if existsSync(nodeModulesDir)
then rmSync(nodeModulesDir, { recursive: true, force: true }) and log the
removal), or if you prefer to change behavior instead, update the CLI docs/help
to remove the “node_modules cache” claim — adjust the code around facetDir and
the logger calls accordingly.
In `@cli/vite-ssr-loader.ts`:
- Around line 109-116: The NODE_PATH concatenation in vite-ssr-loader uses a
hardcoded ':' which breaks on Windows; update the logic that builds
process.env.NODE_PATH (using facetNodeModules and originalNodePath) to join
entries with path.delimiter instead of ':' and ensure path is
imported/available, then call require('module').Module._initPaths() as before to
reload resolution; adjust the ternary that sets process.env.NODE_PATH to use
`${facetNodeModules}${path.delimiter}${originalNodePath}` when originalNodePath
is non-empty.
In `@src/components/Badge.tsx`:
- Around line 432-434: The renderLabel function currently uses a truthy guard
(!label) which hides valid values like 0; update the conditional in renderLabel
to use a nullish check (label == null) so that falsy-but-valid nodes (e.g., 0 or
empty string) are rendered, keeping the Icon behavior unchanged when both are
absent.
In `@src/styles.css`:
- Around line 415-421: The CSS rule ".table-header th" is forcing left alignment
via "text-align: left", which overrides per-column alignment utilities (e.g.,
header cells in LogoGrid.tsx); remove or neutralize that property so alignment
is controlled by component-level classes—either delete the "text-align: left"
declaration or replace it with a non-opinionated value like "text-align:
inherit" on the ".table-header th" selector so per-column utilities can take
effect.
In `@Taskfile.yml`:
- Around line 69-74: The Taskfile.yml task currently describes and runs Bun but
the CLI package uses Vitest; update the task's desc from "Run cli unit tests
with bun" to "Run cli unit tests with Vitest" and replace the cmds entry that
currently runs "bun test" with the proper Vitest invocation—e.g., run "pnpm
test" (keeping the existing dir: cli and deps: gen-version) so the task executes
the CLI package.json "test": "vitest run" command.
---
Nitpick comments:
In @.github/workflows/test.yml:
- Around line 36-40: The workflow still provisions Bun in the "Setup tools" step
(uses: flanksource/deps) by listing bun@v1.3.10 in the tools input; check all
CI/workflow steps and repo scripts (package.json scripts, task files, any
workflow jobs) for references to bun or bun-specific commands, and if none
remain after the Node migration remove bun@v1.3.10 from the tools list (leaving
task@v3.39.2) in the Setup tools step to shorten setup time and eliminate hidden
coupling, then run the CI locally or trigger the workflow to verify no job fails
due to the removal.
In `@cli/test/shell.test.ts`:
- Around line 15-19: Add two regression tests alongside the existing
"shell-escapes interpolated values" test in cli/test/shell.test.ts: one that
verifies array interpolation is escaped correctly by invoking the same $
template (e.g., using an array like ['a','b;echo pwned'] interpolated into
$`printf '%s\n' ${arr}`) and asserting combined stdout matches expected values,
and another that verifies signal-terminated child handling by spawning a process
via the $ template, sending it a signal (SIGTERM/SIGINT) and asserting the
returned result indicates termination by signal (or the promise rejects with the
expected signal info); reuse the existing test harness/quiet() invocation and
assertion style so behavior is locked in for both scalar/array interpolation and
signal termination cases.
🪄 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: 9906502f-d338-4183-8467-04f3bd9550cf
⛔ Files ignored due to path filters (4)
cli/package-lock.jsonis excluded by!**/package-lock.jsoncli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpackage-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (96)
.gavel.yaml.github/workflows/release.yml.github/workflows/test.yml.gitignore.npmrcDockerfileREADME.mdTaskfile.ymlchart/test/.ginkgo/ginkgo-report-.-.-1777281827467563000.jsoncli/.gitignorecli/.npmrccli/ARCHITECTURE.mdcli/README.mdcli/STATUS.mdcli/package.jsoncli/scripts/copy-assets.cjscli/scripts/gen-version.cjscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/live-snapshot.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/generators/html.tscli/src/server/archive.tscli/src/server/facet-cache.tscli/src/server/facet-types.tscli/src/server/preview.tscli/src/server/routes.tscli/src/types.tscli/src/utils/assets.tscli/src/utils/data-loader.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/pdf-generator.tscli/src/utils/remote-resolver.test.tscli/src/utils/remote-resolver.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/pdf-bleed.test.tscli/test/pdf-security.test.tscli/test/render-api.test.tscli/test/resolve-output.test.tscli/test/shell.test.tscli/tsconfig.jsoncli/tsconfig.test.jsoncli/tsdown.config.tscli/vite-dev-loader.tscli/vite-ssr-loader.tscli/vitest.config.tsexamples/kitchen-sink/BadgeExamples.tsxexamples/kitchen-sink/BleedTest.tsxexamples/kitchen-sink/DataFlowDiagram.tsxexamples/kitchen-sink/FindingsReport.tsxexamples/kitchen-sink/FlanksourceHeader.tsxexamples/kitchen-sink/HeaderSolid.tsxexamples/kitchen-sink/PageSizeTest.tsxexamples/kitchen-sink/package.jsonexamples/quickstart/README.mdpackage.jsonsrc/components/Avatar.tsxsrc/components/AvatarGroup.tsxsrc/components/Badge.tsxsrc/components/BulletList.tsxsrc/components/CalloutBox.tsxsrc/components/CompactTable.tsxsrc/components/Document.tsxsrc/components/Finding.tsxsrc/components/Header.tsxsrc/components/ListTable.tsxsrc/components/LogoGrid.tsxsrc/components/MatrixTable.tsxsrc/components/ProgressBar.tsxsrc/components/ScoreGauge.tsxsrc/components/SeverityStatCard.tsxsrc/components/Shield/Shield.tsxsrc/components/StatCard.tsxsrc/components/diagram/Arrow.test.tsxsrc/components/diagram/Arrow.tsxsrc/components/diagram/BoxNode.test.tsxsrc/components/diagram/BoxNode.tsxsrc/components/diagram/Diagram.tsxsrc/components/diagram/colors.tssrc/components/diagram/content.tsxsrc/components/diagram/index.tssrc/components/index.tsxsrc/components/utils/resolveSizeVariant.tssrc/stories/components/Badge.stories.tsxsrc/styles.csstailwind.config.js
💤 Files with no reviewable changes (6)
- cli/test/resolve-output.test.ts
- cli/ARCHITECTURE.md
- cli/STATUS.md
- cli/test/jsx-parser.test.ts
- cli/test/lint-rules.test.ts
- cli/src/server/facet-cache.ts
Comment on lines
+21
to
+22
| pnpm add -g @flanksource/facet | ||
| # or: npm install -g @flanksource/facet |
There was a problem hiding this comment.
Use the CLI package name in global install docs.
Line 21 and Line 22 still install @flanksource/facet, but this migration publishes the CLI as @flanksource/facet-cli. Users following this command can install the wrong package.
Proposed fix
-pnpm add -g `@flanksource/facet`
-# or: npm install -g `@flanksource/facet`
+pnpm add -g `@flanksource/facet-cli`
+# or: npm install -g `@flanksource/facet-cli`📝 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.
Suggested change
| pnpm add -g @flanksource/facet | |
| # or: npm install -g @flanksource/facet | |
| pnpm add -g `@flanksource/facet-cli` | |
| # or: npm install -g `@flanksource/facet-cli` |
🤖 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 `@README.md` around lines 21 - 22, Update the global install examples to use
the new CLI package name by replacing occurrences of the string
"`@flanksource/facet`" with "`@flanksource/facet-cli`" in the README examples (the
two global install lines using "pnpm add -g" and "npm install -g"); ensure both
commands show the new package name so users install the CLI package rather than
the old library package.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/release.yml (2)
111-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDisable persisted git credentials in
build-binariescheckout.This job only needs read access to build/upload artifacts; keep credentials out of local git config by setting
persist-credentials: false.Suggested hardening
- name: Checkout code uses: actions/checkout@v4 with: ref: ${{ needs.version-bump.outputs.new_tag }} + persist-credentials: false🤖 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/release.yml around lines 111 - 115, The checkout step using actions/checkout@v4 in the build-binaries job is persisting git credentials; update that step (the Checkout code step) to include persist-credentials: false so credentials are not stored in the local git config. Locate the step that uses actions/checkout@v4 and add the with key persist-credentials: false alongside the existing ref: ${{ needs.version-bump.outputs.new_tag }}.Source: Linters/SAST tools
112-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin all changed
uses:actions to immutable commit SHAs.These steps still use mutable tags (
@v4,@v1.0.24), which weakens workflow supply-chain integrity. Pin each action reference to a full commit SHA.#!/bin/bash set -euo pipefail # Expectation after fix: no output lines from this workflow. rg -n '^\s*uses:\s*[^#\s]+@v?[0-9]+(\.[0-9]+)?(\.[0-9]+)?\s*$' .github/workflows/release.ymlAlso applies to: 117-117, 122-122, 127-127, 149-149, 161-161
🤖 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/release.yml at line 112, Several workflow steps still reference mutable action tags (e.g. uses: actions/checkout@v4 and other uses: lines flagged), so replace each mutable tag with the corresponding action's immutable full commit SHA (40-char) — for example swap uses: actions/checkout@v4 to uses: actions/checkout@<full-commit-sha> and do the same for actions/setup-node, aws-actions/configure-aws-credentials, aws-actions/amazon-ecr-login, actions/upload-artifact (and any other uses: entries flagged). Retrieve the correct SHA from the action's GitHub repo (select the commit that matches the tag you want) and update the uses: values to the full SHA, then run the provided regex check to ensure there are no remaining numeric tags.Source: Linters/SAST tools
🤖 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.
Duplicate comments:
In @.github/workflows/release.yml:
- Around line 111-115: The checkout step using actions/checkout@v4 in the
build-binaries job is persisting git credentials; update that step (the Checkout
code step) to include persist-credentials: false so credentials are not stored
in the local git config. Locate the step that uses actions/checkout@v4 and add
the with key persist-credentials: false alongside the existing ref: ${{
needs.version-bump.outputs.new_tag }}.
- Line 112: Several workflow steps still reference mutable action tags (e.g.
uses: actions/checkout@v4 and other uses: lines flagged), so replace each
mutable tag with the corresponding action's immutable full commit SHA (40-char)
— for example swap uses: actions/checkout@v4 to uses:
actions/checkout@<full-commit-sha> and do the same for actions/setup-node,
aws-actions/configure-aws-credentials, aws-actions/amazon-ecr-login,
actions/upload-artifact (and any other uses: entries flagged). Retrieve the
correct SHA from the action's GitHub repo (select the commit that matches the
tag you want) and update the uses: values to the full SHA, then run the provided
regex check to ensure there are no remaining numeric tags.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 016c5db9-2603-41c9-ad57-597dcd938007
⛔ Files ignored due to path filters (1)
cli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
.github/workflows/release.ymlcli/package.jsoncli/src/bundler/live-snapshot.tscli/src/cli.bun.tscli/src/server/worker-pool.tscli/src/utils/assets.bun.tscli/src/utils/assets.tscli/src/utils/debug-annotations.tscli/src/utils/pdf-generator.tscli/src/utils/pdf-multipass.tscli/tsconfig.jsoncli/tsdown.config.ts
✅ Files skipped from review due to trivial changes (3)
- cli/src/cli.bun.ts
- cli/src/utils/pdf-multipass.ts
- cli/src/utils/debug-annotations.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- cli/src/utils/pdf-generator.ts
- cli/tsdown.config.ts
- cli/tsconfig.json
- cli/src/bundler/live-snapshot.ts
moshloop
force-pushed
the
refactor/cli-bun-to-node
branch
2 times, most recently
from
bef4f06 to
c6353f2
Compare
…peteer-core
Rebased onto main (0.1.41), dropping commits main already absorbed
(pnpm migration, component work) and re-applying the bun->node migration
on top of main's newer CLI code.
CLI runtime migration:
- Node $ shell helper (utils/shell.ts) replaces Bun shell across the CLI,
including main's newer files (package-manager.ts, vite-builder.ts,
doctor.ts, routes.ts, generators/html.ts, remote-resolver.ts, etc.)
- node:http server with Web Request/Response adapters replaces Bun.serve
- node:child_process (spawn/execFileSync) replaces Bun.spawn*; tsx runs
the vite loaders via resolveTsxBin (no bun runtime)
- assets resolved via import.meta.url (utils/assets.ts) instead of Bun's
`with { type: 'file' }`; all tests run under vitest, not bun:test
Distribution:
- published as @flanksource/facet-cli (Node ESM bundle, dist/cli.mjs bin)
- standalone bun binary restored via build:bin -> src/cli.bun.ts, which
embeds assets (cli.bun.ts/assets.bun.ts use bun's file imports) so the
compiled binary resolves them from /$bunfs; release.yml builds 3 targets
and publishes both @flanksource/facet and @flanksource/facet-cli
puppeteer -> puppeteer-core:
- no Chromium auto-download; always use system Chrome via resolveChromePath()
Render cache:
- configurable cacheDir (--cache-dir / FACET_CACHE_DIR); render-api test
uses an isolated temp cache so the /render/stream test renders cold
Verified: node bundle and bun binary both render SimpleReport.tsx to a
valid PDF-1.7; vitest 203 passed; tsc 11 errors (all pre-existing on main).
yashmehrotra
force-pushed
the
refactor/cli-bun-to-node
branch
from
c6353f2 to
a8b51d5
Compare
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/bundler/vite-builder.ts (1)
61-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid raw shell concatenation for loader execution.
runLoaderOnce()injects bothsrtPrefix()andresolveTsxBin(...).join(' ')as raw shell text. This breaks valid paths with spaces and can alter command parsing when paths contain shell metacharacters. Build an argv token list and quote each token (or execute viaspawn/execFile) instead of joining raw strings.Proposed fix
+function shellQuote(arg: string): string { + return `'${arg.replace(/'/g, `'\\''`)}'`; +} + -function srtPrefix(sandbox?: string | boolean): string { - if (!sandbox) return ''; - const settings = typeof sandbox === 'string' ? sandbox : '/etc/facet/srt-settings.json'; - return `srt --settings ${settings} `; +function srtPrefix(sandbox?: string | boolean): string[] { + if (!sandbox) return []; + const settings = typeof sandbox === 'string' ? sandbox : '/etc/facet/srt-settings.json'; + return ['srt', '--settings', settings]; } @@ async function runLoaderOnce(args: LoaderArgs): Promise<{ stderr?: Buffer | string; }> { - const prefix = srtPrefix(args.sandbox); - const tsx = resolveTsxBin(args.facetRoot).join(' '); - return await $`${{ raw: prefix }}${{ raw: tsx }} ${args.loaderPath} --facet-root=${args.facetRoot} --data-file=${args.dataFilePath} --output-file=${args.resultFilePath}`.quiet(); + const argv = [ + ...srtPrefix(args.sandbox), + ...resolveTsxBin(args.facetRoot), + args.loaderPath, + `--facet-root=${args.facetRoot}`, + `--data-file=${args.dataFilePath}`, + `--output-file=${args.resultFilePath}`, + ]; + const cmd = argv.map(shellQuote).join(' '); + return await $`${{ raw: cmd }}`.quiet(); }Also applies to: 139-143
🤖 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-builder.ts` around lines 61 - 65, The srtPrefix function and the resolveTsxBin call are both producing raw strings that get concatenated for shell execution in runLoaderOnce(), which fails when paths contain spaces or shell metacharacters. Instead of concatenating raw strings, refactor srtPrefix to return an array of command tokens (e.g., ['srt', '--settings', settings]) and similarly refactor how resolveTsxBin is handled to build an argv array. Then pass this argv array to spawn or execFile instead of using shell string concatenation, or if using shell execution is necessary, properly quote each token in the final command string.
🧹 Nitpick comments (3)
cli/src/commands/doctor.ts (1)
484-491: 💤 Low valueArray interpolation in shell template may produce malformed commands.
The template
`${name} ${args}`converts theargsarray via.toString(), which joins elements with commas. For multi-element arrays (e.g.,['--version', '--quiet']), this producesgit --version,--quietrather thangit --version --quiet. Current call sites use single-element arrays so this works, but it's fragile.🛠️ Safer alternative using join
async function probeOptionalBin(name: string, args: string[]): Promise<{ ok: true; output: string } | { ok: false }> { try { - const out = await $`${name} ${args}`.quiet(); + const out = await $`${name} ${args.join(' ')}`.quiet(); return { ok: true, output: out.stdout.toString().trim() }; } catch { return { ok: false }; } }🤖 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/commands/doctor.ts` around lines 484 - 491, The probeOptionalBin function interpolates the args array directly into the shell template, which calls .toString() on the array and joins elements with commas rather than spaces, potentially producing malformed commands like "git --version,--quiet" instead of "git --version --quiet". Fix this by joining the args array with spaces before interpolating it into the template literal so that multi-element arrays are properly formatted as separate command arguments..github/workflows/release.yml (1)
35-35: ⚡ Quick winNode version inconsistency across jobs.
version-bumpandbuild-binariesuse Node 20 whilepublish-npmuses Node 22. This inconsistency could cause subtle issues with lockfile formats or behavior differences. Consider aligning all jobs to the same Node version.♻️ Align Node versions
- name: Setup Node.js uses: actions/setup-node@v4 with: - node-version: "22" + node-version: "20" registry-url: "https://registry.npmjs.org"Or update all jobs to Node 22 if that's the intended target.
Also applies to: 85-85, 178-178
🤖 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/release.yml at line 35, The GitHub Actions workflow file has inconsistent Node versions across multiple jobs: the version-bump job uses Node 20 at line 35, the build-binaries job uses Node 20 at line 85, and the publish-npm job uses Node 22 at line 178. Update all three node-version specifications to use the same version (either all to Node 20 or all to Node 22) to ensure consistent behavior and avoid potential lockfile format or behavior differences across the build pipeline.cli/src/utils/pdf-generator.ts (1)
15-15: Add explicit browser-path preflight check to fail fast with clear diagnostics when usingpuppeteer-coreWhen
puppeteer-coreis unable to resolve a browser path, it will fail at runtime with a generic error rather than a clear preflight diagnostic. Additionally, the current fallback message "Puppeteer bundled Chromium" is misleading sincepuppeteer-coreincludes no bundled binary and requires an explicitexecutablePathto launch.Implement the validation in both
launchBrowser()(line 473) and inline at line 421 to catch missing Chrome/Chromium paths immediately with the suggested error message. This improves diagnostics and guides users toward resolution.Proposed adjustment
const chromePath = resolveChromePath(); - log.debug(`Using browser: ${chromePath ?? 'Puppeteer bundled Chromium'}`); + if (!chromePath) { + throw new Error( + 'No Chromium/Chrome binary found. Set PUPPETEER_EXECUTABLE_PATH or CHROME_PATH, or install one via `npx puppeteer browsers install chrome`.' + ); + } + log.debug(`Using browser: ${chromePath}`); const browser = await puppeteer.launch({ headless: true, executablePath: chromePath, args: ['--no-sandbox', '--disable-setuid-sandbox'], });🤖 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-generator.ts` at line 15, Add explicit browser-path preflight validation to fail fast when using puppeteer-core without an available browser executable. In the launchBrowser() function (around line 473) and inline at line 421, implement checks to verify that a valid Chrome/Chromium executable path is available before attempting to launch. Replace the misleading fallback message "Puppeteer bundled Chromium" with a clear error that explains puppeteer-core requires an explicit executablePath configuration. This ensures missing browser paths are caught immediately with diagnostic information that guides users toward resolution, rather than failing at runtime with a generic error.
🤖 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:
- Around line 72-75: The checkout step in the build-binaries job should add the
`persist-credentials: false` parameter to the actions/checkout@v4 action
(currently at lines 72-75). This parameter disables Git credential persistence
since the build-binaries job only reads the tagged commit and uploads artifacts
without performing any Git push operations, following the least-privilege
security principle.
In `@cli/package.json`:
- Line 22: The build:bin script in cli/package.json is unused dead code that
contradicts the PR's migration away from Bun. Since there are no references to
this script anywhere in the codebase, remove the entire line defining build:bin
from the scripts section of cli/package.json.
In `@cli/src/bundler/vite-builder.ts`:
- Around line 118-121: The `/ERR_MODULE_NOT_FOUND/` regex pattern in
RUNTIME_REINSTALL_PATTERNS is too broad and incorrectly matches relative-path
template errors in addition to package-miss cases, causing unnecessary
reinstalls. Replace this catch-all regex with a more specific pattern that
matches the exact package-miss error wording only, excluding relative-path
misses. Additionally, update the corresponding test assertion for
ERR_MODULE_NOT_FOUND in the cli/src/bundler/vite-builder.test.ts file to
validate the narrowed behavior and ensure it no longer triggers reinstalls for
relative-path errors.
In `@cli/src/utils/package-manager.ts`:
- Line 11: The shell module being imported here uses an unconditional `/bin/sh`
path that does not exist on Windows, causing ENOENT errors when executing
commands like pnpm version checks. In the shell module being imported, replace
the hardcoded `/bin/sh` with cross-platform compatible shell detection that uses
`cmd.exe` on Windows systems and `/bin/sh` on Unix-like systems. This will
ensure the $ function works correctly across all platforms and fix the broken
pnpm resolution flows in vite-builder, template resolution, and doctor commands
that depend on resolvePackageManager().
In `@cli/test/pdf-security.test.ts`:
- Line 13: The assignment of process.env['FACET_PACKAGE_PATH'] at module scope
in cli/test/pdf-security.test.ts at line 13 and cli/test/render-api.test.ts at
line 21 causes environment pollution for subsequent tests in the same worker
process. For each file, save the previous value of
process.env['FACET_PACKAGE_PATH'] before the assignment (store it in a const
like savedPath), and add or update an afterAll block to restore the original
state by either deleting the environment variable if it was previously
undefined, or restoring the saved value if it existed.
---
Outside diff comments:
In `@cli/src/bundler/vite-builder.ts`:
- Around line 61-65: The srtPrefix function and the resolveTsxBin call are both
producing raw strings that get concatenated for shell execution in
runLoaderOnce(), which fails when paths contain spaces or shell metacharacters.
Instead of concatenating raw strings, refactor srtPrefix to return an array of
command tokens (e.g., ['srt', '--settings', settings]) and similarly refactor
how resolveTsxBin is handled to build an argv array. Then pass this argv array
to spawn or execFile instead of using shell string concatenation, or if using
shell execution is necessary, properly quote each token in the final command
string.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 35: The GitHub Actions workflow file has inconsistent Node versions
across multiple jobs: the version-bump job uses Node 20 at line 35, the
build-binaries job uses Node 20 at line 85, and the publish-npm job uses Node 22
at line 178. Update all three node-version specifications to use the same
version (either all to Node 20 or all to Node 22) to ensure consistent behavior
and avoid potential lockfile format or behavior differences across the build
pipeline.
In `@cli/src/commands/doctor.ts`:
- Around line 484-491: The probeOptionalBin function interpolates the args array
directly into the shell template, which calls .toString() on the array and joins
elements with commas rather than spaces, potentially producing malformed
commands like "git --version,--quiet" instead of "git --version --quiet". Fix
this by joining the args array with spaces before interpolating it into the
template literal so that multi-element arrays are properly formatted as separate
command arguments.
In `@cli/src/utils/pdf-generator.ts`:
- Line 15: Add explicit browser-path preflight validation to fail fast when
using puppeteer-core without an available browser executable. In the
launchBrowser() function (around line 473) and inline at line 421, implement
checks to verify that a valid Chrome/Chromium executable path is available
before attempting to launch. Replace the misleading fallback message "Puppeteer
bundled Chromium" with a clear error that explains puppeteer-core requires an
explicit executablePath configuration. This ensures missing browser paths are
caught immediately with diagnostic information that guides users toward
resolution, rather than failing at runtime with a generic error.
🪄 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: f8de4e19-1826-41e8-9f42-1563fe819fdc
⛔ Files ignored due to path filters (2)
cli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
.github/workflows/release.yml.gitignoreDockerfilecli/.gitignorecli/README.mdcli/package.jsoncli/scripts/copy-assets.cjscli/scripts/gen-version.cjscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/live-snapshot.tscli/src/bundler/vite-builder.test.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/cli.bun.tscli/src/cli.tscli/src/commands/doctor.test.tscli/src/commands/doctor.tscli/src/generators/html.tscli/src/server/archive.tscli/src/server/config.tscli/src/server/preview.tscli/src/server/routes.tscli/src/server/worker-pool.tscli/src/types.tscli/src/utils/assets.bun.tscli/src/utils/assets.tscli/src/utils/data-loader.tscli/src/utils/debug-annotations.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/pdf-generator.tscli/src/utils/pdf-multipass.tscli/src/utils/remote-resolver.test.tscli/src/utils/remote-resolver.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/pdf-bleed.test.tscli/test/pdf-security.test.tscli/test/render-api.test.tscli/test/resolve-output.test.tscli/test/shell.test.tscli/tsconfig.jsoncli/tsconfig.test.jsoncli/tsdown.config.tscli/vite-dev-loader.tscli/vite-ssr-loader.tscli/vitest.config.tsexamples/kitchen-sink/BleedTest.tsxexamples/kitchen-sink/DataFlowDiagram.tsxexamples/kitchen-sink/PageSizeTest.tsxpackage.jsonsrc/components/CompactTable.tsxsrc/components/LogoGrid.tsxsrc/components/MatrixTable.tsxsrc/components/StatCard.tsxsrc/components/diagram/Arrow.test.tsxsrc/components/diagram/Arrow.tsxsrc/components/diagram/BoxNode.test.tsxsrc/components/diagram/BoxNode.tsxsrc/components/diagram/Diagram.tsxsrc/components/diagram/colors.tssrc/components/diagram/content.tsxsrc/components/diagram/index.tssrc/components/index.tsx
💤 Files with no reviewable changes (2)
- cli/test/lint-rules.test.ts
- cli/test/jsx-parser.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/components/CompactTable.tsx
- cli/vitest.config.ts
- cli/src/utils/pdf-multipass.ts
- cli/src/server/worker-pool.ts
- cli/src/utils/debug-annotations.ts
- src/components/LogoGrid.tsx
- src/components/diagram/index.ts
- cli/scripts/gen-version.cjs
🚧 Files skipped from review as they are similar to previous changes (38)
- .gitignore
- cli/src/server/archive.ts
- src/components/diagram/colors.ts
- cli/src/types.ts
- cli/src/cli.bun.ts
- cli/tsconfig.test.json
- cli/.gitignore
- src/components/diagram/content.tsx
- cli/scripts/copy-assets.cjs
- src/components/diagram/Arrow.test.tsx
- cli/src/utils/assets.bun.ts
- cli/src/utils/assets.ts
- cli/test/pdf-bleed.test.ts
- cli/src/utils/data-loader.ts
- src/components/diagram/Diagram.tsx
- src/components/index.tsx
- cli/src/commands/doctor.test.ts
- examples/kitchen-sink/PageSizeTest.tsx
- src/components/MatrixTable.tsx
- cli/tsdown.config.ts
- cli/src/cli.ts
- src/components/diagram/BoxNode.test.tsx
- src/components/diagram/BoxNode.tsx
- cli/src/utils/tailwind.ts
- cli/src/utils/package-manager.test.ts
- cli/src/utils/shell.ts
- src/components/diagram/Arrow.tsx
- cli/src/utils/remote-resolver.test.ts
- cli/README.md
- cli/src/bundler/vite-server.ts
- cli/test/shell.test.ts
- cli/src/server/routes.ts
- cli/src/utils/tailwind.test.ts
- Dockerfile
- cli/vite-ssr-loader.ts
- cli/src/server/preview.ts
- cli/src/bundler/live-snapshot.ts
- cli/src/builders/facet-directory.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/bundler/vite-builder.ts (1)
61-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid raw shell concatenation for loader execution.
runLoaderOnce()injects bothsrtPrefix()andresolveTsxBin(...).join(' ')as raw shell text. This breaks valid paths with spaces and can alter command parsing when paths contain shell metacharacters. Build an argv token list and quote each token (or execute viaspawn/execFile) instead of joining raw strings.Proposed fix
+function shellQuote(arg: string): string { + return `'${arg.replace(/'/g, `'\\''`)}'`; +} + -function srtPrefix(sandbox?: string | boolean): string { - if (!sandbox) return ''; - const settings = typeof sandbox === 'string' ? sandbox : '/etc/facet/srt-settings.json'; - return `srt --settings ${settings} `; +function srtPrefix(sandbox?: string | boolean): string[] { + if (!sandbox) return []; + const settings = typeof sandbox === 'string' ? sandbox : '/etc/facet/srt-settings.json'; + return ['srt', '--settings', settings]; } @@ async function runLoaderOnce(args: LoaderArgs): Promise<{ stderr?: Buffer | string; }> { - const prefix = srtPrefix(args.sandbox); - const tsx = resolveTsxBin(args.facetRoot).join(' '); - return await $`${{ raw: prefix }}${{ raw: tsx }} ${args.loaderPath} --facet-root=${args.facetRoot} --data-file=${args.dataFilePath} --output-file=${args.resultFilePath}`.quiet(); + const argv = [ + ...srtPrefix(args.sandbox), + ...resolveTsxBin(args.facetRoot), + args.loaderPath, + `--facet-root=${args.facetRoot}`, + `--data-file=${args.dataFilePath}`, + `--output-file=${args.resultFilePath}`, + ]; + const cmd = argv.map(shellQuote).join(' '); + return await $`${{ raw: cmd }}`.quiet(); }Also applies to: 139-143
🤖 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-builder.ts` around lines 61 - 65, The srtPrefix function and the resolveTsxBin call are both producing raw strings that get concatenated for shell execution in runLoaderOnce(), which fails when paths contain spaces or shell metacharacters. Instead of concatenating raw strings, refactor srtPrefix to return an array of command tokens (e.g., ['srt', '--settings', settings]) and similarly refactor how resolveTsxBin is handled to build an argv array. Then pass this argv array to spawn or execFile instead of using shell string concatenation, or if using shell execution is necessary, properly quote each token in the final command string.
🧹 Nitpick comments (3)
cli/src/commands/doctor.ts (1)
484-491: 💤 Low valueArray interpolation in shell template may produce malformed commands.
The template
`${name} ${args}`converts theargsarray via.toString(), which joins elements with commas. For multi-element arrays (e.g.,['--version', '--quiet']), this producesgit --version,--quietrather thangit --version --quiet. Current call sites use single-element arrays so this works, but it's fragile.🛠️ Safer alternative using join
async function probeOptionalBin(name: string, args: string[]): Promise<{ ok: true; output: string } | { ok: false }> { try { - const out = await $`${name} ${args}`.quiet(); + const out = await $`${name} ${args.join(' ')}`.quiet(); return { ok: true, output: out.stdout.toString().trim() }; } catch { return { ok: false }; } }🤖 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/commands/doctor.ts` around lines 484 - 491, The probeOptionalBin function interpolates the args array directly into the shell template, which calls .toString() on the array and joins elements with commas rather than spaces, potentially producing malformed commands like "git --version,--quiet" instead of "git --version --quiet". Fix this by joining the args array with spaces before interpolating it into the template literal so that multi-element arrays are properly formatted as separate command arguments..github/workflows/release.yml (1)
35-35: ⚡ Quick winNode version inconsistency across jobs.
version-bumpandbuild-binariesuse Node 20 whilepublish-npmuses Node 22. This inconsistency could cause subtle issues with lockfile formats or behavior differences. Consider aligning all jobs to the same Node version.♻️ Align Node versions
- name: Setup Node.js uses: actions/setup-node@v4 with: - node-version: "22" + node-version: "20" registry-url: "https://registry.npmjs.org"Or update all jobs to Node 22 if that's the intended target.
Also applies to: 85-85, 178-178
🤖 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/release.yml at line 35, The GitHub Actions workflow file has inconsistent Node versions across multiple jobs: the version-bump job uses Node 20 at line 35, the build-binaries job uses Node 20 at line 85, and the publish-npm job uses Node 22 at line 178. Update all three node-version specifications to use the same version (either all to Node 20 or all to Node 22) to ensure consistent behavior and avoid potential lockfile format or behavior differences across the build pipeline.cli/src/utils/pdf-generator.ts (1)
15-15: Add explicit browser-path preflight check to fail fast with clear diagnostics when usingpuppeteer-coreWhen
puppeteer-coreis unable to resolve a browser path, it will fail at runtime with a generic error rather than a clear preflight diagnostic. Additionally, the current fallback message "Puppeteer bundled Chromium" is misleading sincepuppeteer-coreincludes no bundled binary and requires an explicitexecutablePathto launch.Implement the validation in both
launchBrowser()(line 473) and inline at line 421 to catch missing Chrome/Chromium paths immediately with the suggested error message. This improves diagnostics and guides users toward resolution.Proposed adjustment
const chromePath = resolveChromePath(); - log.debug(`Using browser: ${chromePath ?? 'Puppeteer bundled Chromium'}`); + if (!chromePath) { + throw new Error( + 'No Chromium/Chrome binary found. Set PUPPETEER_EXECUTABLE_PATH or CHROME_PATH, or install one via `npx puppeteer browsers install chrome`.' + ); + } + log.debug(`Using browser: ${chromePath}`); const browser = await puppeteer.launch({ headless: true, executablePath: chromePath, args: ['--no-sandbox', '--disable-setuid-sandbox'], });🤖 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-generator.ts` at line 15, Add explicit browser-path preflight validation to fail fast when using puppeteer-core without an available browser executable. In the launchBrowser() function (around line 473) and inline at line 421, implement checks to verify that a valid Chrome/Chromium executable path is available before attempting to launch. Replace the misleading fallback message "Puppeteer bundled Chromium" with a clear error that explains puppeteer-core requires an explicit executablePath configuration. This ensures missing browser paths are caught immediately with diagnostic information that guides users toward resolution, rather than failing at runtime with a generic error.
🤖 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:
- Around line 72-75: The checkout step in the build-binaries job should add the
`persist-credentials: false` parameter to the actions/checkout@v4 action
(currently at lines 72-75). This parameter disables Git credential persistence
since the build-binaries job only reads the tagged commit and uploads artifacts
without performing any Git push operations, following the least-privilege
security principle.
In `@cli/package.json`:
- Line 22: The build:bin script in cli/package.json is unused dead code that
contradicts the PR's migration away from Bun. Since there are no references to
this script anywhere in the codebase, remove the entire line defining build:bin
from the scripts section of cli/package.json.
In `@cli/src/bundler/vite-builder.ts`:
- Around line 118-121: The `/ERR_MODULE_NOT_FOUND/` regex pattern in
RUNTIME_REINSTALL_PATTERNS is too broad and incorrectly matches relative-path
template errors in addition to package-miss cases, causing unnecessary
reinstalls. Replace this catch-all regex with a more specific pattern that
matches the exact package-miss error wording only, excluding relative-path
misses. Additionally, update the corresponding test assertion for
ERR_MODULE_NOT_FOUND in the cli/src/bundler/vite-builder.test.ts file to
validate the narrowed behavior and ensure it no longer triggers reinstalls for
relative-path errors.
In `@cli/src/utils/package-manager.ts`:
- Line 11: The shell module being imported here uses an unconditional `/bin/sh`
path that does not exist on Windows, causing ENOENT errors when executing
commands like pnpm version checks. In the shell module being imported, replace
the hardcoded `/bin/sh` with cross-platform compatible shell detection that uses
`cmd.exe` on Windows systems and `/bin/sh` on Unix-like systems. This will
ensure the $ function works correctly across all platforms and fix the broken
pnpm resolution flows in vite-builder, template resolution, and doctor commands
that depend on resolvePackageManager().
In `@cli/test/pdf-security.test.ts`:
- Line 13: The assignment of process.env['FACET_PACKAGE_PATH'] at module scope
in cli/test/pdf-security.test.ts at line 13 and cli/test/render-api.test.ts at
line 21 causes environment pollution for subsequent tests in the same worker
process. For each file, save the previous value of
process.env['FACET_PACKAGE_PATH'] before the assignment (store it in a const
like savedPath), and add or update an afterAll block to restore the original
state by either deleting the environment variable if it was previously
undefined, or restoring the saved value if it existed.
---
Outside diff comments:
In `@cli/src/bundler/vite-builder.ts`:
- Around line 61-65: The srtPrefix function and the resolveTsxBin call are both
producing raw strings that get concatenated for shell execution in
runLoaderOnce(), which fails when paths contain spaces or shell metacharacters.
Instead of concatenating raw strings, refactor srtPrefix to return an array of
command tokens (e.g., ['srt', '--settings', settings]) and similarly refactor
how resolveTsxBin is handled to build an argv array. Then pass this argv array
to spawn or execFile instead of using shell string concatenation, or if using
shell execution is necessary, properly quote each token in the final command
string.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Line 35: The GitHub Actions workflow file has inconsistent Node versions
across multiple jobs: the version-bump job uses Node 20 at line 35, the
build-binaries job uses Node 20 at line 85, and the publish-npm job uses Node 22
at line 178. Update all three node-version specifications to use the same
version (either all to Node 20 or all to Node 22) to ensure consistent behavior
and avoid potential lockfile format or behavior differences across the build
pipeline.
In `@cli/src/commands/doctor.ts`:
- Around line 484-491: The probeOptionalBin function interpolates the args array
directly into the shell template, which calls .toString() on the array and joins
elements with commas rather than spaces, potentially producing malformed
commands like "git --version,--quiet" instead of "git --version --quiet". Fix
this by joining the args array with spaces before interpolating it into the
template literal so that multi-element arrays are properly formatted as separate
command arguments.
In `@cli/src/utils/pdf-generator.ts`:
- Line 15: Add explicit browser-path preflight validation to fail fast when
using puppeteer-core without an available browser executable. In the
launchBrowser() function (around line 473) and inline at line 421, implement
checks to verify that a valid Chrome/Chromium executable path is available
before attempting to launch. Replace the misleading fallback message "Puppeteer
bundled Chromium" with a clear error that explains puppeteer-core requires an
explicit executablePath configuration. This ensures missing browser paths are
caught immediately with diagnostic information that guides users toward
resolution, rather than failing at runtime with a generic error.
🪄 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: f8de4e19-1826-41e8-9f42-1563fe819fdc
⛔ Files ignored due to path filters (2)
cli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (68)
.github/workflows/release.yml.gitignoreDockerfilecli/.gitignorecli/README.mdcli/package.jsoncli/scripts/copy-assets.cjscli/scripts/gen-version.cjscli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/live-snapshot.tscli/src/bundler/vite-builder.test.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/cli.bun.tscli/src/cli.tscli/src/commands/doctor.test.tscli/src/commands/doctor.tscli/src/generators/html.tscli/src/server/archive.tscli/src/server/config.tscli/src/server/preview.tscli/src/server/routes.tscli/src/server/worker-pool.tscli/src/types.tscli/src/utils/assets.bun.tscli/src/utils/assets.tscli/src/utils/data-loader.tscli/src/utils/debug-annotations.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/pdf-generator.tscli/src/utils/pdf-multipass.tscli/src/utils/remote-resolver.test.tscli/src/utils/remote-resolver.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/pdf-bleed.test.tscli/test/pdf-security.test.tscli/test/render-api.test.tscli/test/resolve-output.test.tscli/test/shell.test.tscli/tsconfig.jsoncli/tsconfig.test.jsoncli/tsdown.config.tscli/vite-dev-loader.tscli/vite-ssr-loader.tscli/vitest.config.tsexamples/kitchen-sink/BleedTest.tsxexamples/kitchen-sink/DataFlowDiagram.tsxexamples/kitchen-sink/PageSizeTest.tsxpackage.jsonsrc/components/CompactTable.tsxsrc/components/LogoGrid.tsxsrc/components/MatrixTable.tsxsrc/components/StatCard.tsxsrc/components/diagram/Arrow.test.tsxsrc/components/diagram/Arrow.tsxsrc/components/diagram/BoxNode.test.tsxsrc/components/diagram/BoxNode.tsxsrc/components/diagram/Diagram.tsxsrc/components/diagram/colors.tssrc/components/diagram/content.tsxsrc/components/diagram/index.tssrc/components/index.tsx
💤 Files with no reviewable changes (2)
- cli/test/lint-rules.test.ts
- cli/test/jsx-parser.test.ts
✅ Files skipped from review due to trivial changes (8)
- src/components/CompactTable.tsx
- cli/vitest.config.ts
- cli/src/utils/pdf-multipass.ts
- cli/src/server/worker-pool.ts
- cli/src/utils/debug-annotations.ts
- src/components/LogoGrid.tsx
- src/components/diagram/index.ts
- cli/scripts/gen-version.cjs
🚧 Files skipped from review as they are similar to previous changes (38)
- .gitignore
- cli/src/server/archive.ts
- src/components/diagram/colors.ts
- cli/src/types.ts
- cli/src/cli.bun.ts
- cli/tsconfig.test.json
- cli/.gitignore
- src/components/diagram/content.tsx
- cli/scripts/copy-assets.cjs
- src/components/diagram/Arrow.test.tsx
- cli/src/utils/assets.bun.ts
- cli/src/utils/assets.ts
- cli/test/pdf-bleed.test.ts
- cli/src/utils/data-loader.ts
- src/components/diagram/Diagram.tsx
- src/components/index.tsx
- cli/src/commands/doctor.test.ts
- examples/kitchen-sink/PageSizeTest.tsx
- src/components/MatrixTable.tsx
- cli/tsdown.config.ts
- cli/src/cli.ts
- src/components/diagram/BoxNode.test.tsx
- src/components/diagram/BoxNode.tsx
- cli/src/utils/tailwind.ts
- cli/src/utils/package-manager.test.ts
- cli/src/utils/shell.ts
- src/components/diagram/Arrow.tsx
- cli/src/utils/remote-resolver.test.ts
- cli/README.md
- cli/src/bundler/vite-server.ts
- cli/test/shell.test.ts
- cli/src/server/routes.ts
- cli/src/utils/tailwind.test.ts
- Dockerfile
- cli/vite-ssr-loader.ts
- cli/src/server/preview.ts
- cli/src/bundler/live-snapshot.ts
- cli/src/builders/facet-directory.ts
🛑 Comments failed to post (5)
.github/workflows/release.yml (1)
72-75:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDisable credential persistence in build-binaries checkout.
The build-binaries job only reads the tagged commit and uploads artifacts; it doesn't push to Git. Add
persist-credentials: falseto follow least-privilege principles consistent with the publish-npm job.🔐 Suggested fix
- name: Checkout code uses: actions/checkout@v4 with: ref: ${{ needs.version-bump.outputs.new_tag }} + persist-credentials: false📝 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.- name: Checkout code uses: actions/checkout@v4 with: ref: ${{ needs.version-bump.outputs.new_tag }} persist-credentials: false🧰 Tools
🪛 zizmor (1.25.2)
[warning] 72-75: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 73-73: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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/release.yml around lines 72 - 75, The checkout step in the build-binaries job should add the `persist-credentials: false` parameter to the actions/checkout@v4 action (currently at lines 72-75). This parameter disables Git credential persistence since the build-binaries job only reads the tagged commit and uploads artifacts without performing any Git push operations, following the least-privilege security principle.Source: Linters/SAST tools
cli/package.json (1)
22-22:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether `build:bin` (or bun build) is still referenced by docs/workflows/scripts. rg -n -C2 '"build:bin"|bun build|pnpm run build:bin|npm run build:bin'Repository: flanksource/facet
Length of output: 779
Remove the unused
build:binscript.Line 22 defines a Bun-only script that contradicts the PR objective to migrate off Bun. Verification found no references to
build:binin the codebase—it is dead code. Remove it entirely or gate it with a clear comment explaining it is deprecated.🤖 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/package.json` at line 22, The build:bin script in cli/package.json is unused dead code that contradicts the PR's migration away from Bun. Since there are no references to this script anywhere in the codebase, remove the entire line defining build:bin from the scripts section of cli/package.json.cli/src/bundler/vite-builder.ts (1)
118-121:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow
ERR_MODULE_NOT_FOUNDmatching to package-miss cases only.The catch-all
/ERR_MODULE_NOT_FOUND/triggers reinstall for relative-path template errors too, which defeats the package-only intent and causes unnecessary.facetnukes/reinstalls. Match package-miss wording explicitly and keep relative-path misses out.Proposed fix
const RUNTIME_REINSTALL_PATTERNS = [ /Cannot find module ['"][^.\/][^'"]*['"]/i, - /ERR_MODULE_NOT_FOUND/, + /Cannot find package ['"][^.\/][^'"]*['"]/i, /Failed to resolve import ['"][^.\/][^'"]*['"]/i, /Failed to resolve entry/i, /Could not resolve ['"][^.\/][^'"]*['"] from/i, /Rollup failed to resolve(?: import)? ['"][^.\/][^'"]*['"]/i, ];Also update the
cli/src/bundler/vite-builder.test.tsERR_MODULE_NOT_FOUNDassertion to reflect the narrowed behavior.📝 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.const RUNTIME_REINSTALL_PATTERNS = [ /Cannot find module ['"][^.\/][^'"]*['"]/i, /Cannot find package ['"][^.\/][^'"]*['"]/i, /Failed to resolve import ['"][^.\/][^'"]*['"]/i,🤖 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-builder.ts` around lines 118 - 121, The `/ERR_MODULE_NOT_FOUND/` regex pattern in RUNTIME_REINSTALL_PATTERNS is too broad and incorrectly matches relative-path template errors in addition to package-miss cases, causing unnecessary reinstalls. Replace this catch-all regex with a more specific pattern that matches the exact package-miss error wording only, excluding relative-path misses. Additionally, update the corresponding test assertion for ERR_MODULE_NOT_FOUND in the cli/src/bundler/vite-builder.test.ts file to validate the narrowed behavior and ensure it no longer triggers reinstalls for relative-path errors.cli/src/utils/package-manager.ts (1)
11-11:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether the shell helper is POSIX-only and where this resolver depends on it. set -euo pipefail echo "== shell helper implementation ==" fd -i 'shell.ts' cli/src --exec sed -n '1,240p' {} echo echo "== shell helper platform assumptions ==" rg -n "(/bin/sh|cmd\\.exe|powershell|spawn\\(|execFile\\()" cli/src/utils/shell.ts -n -C 3 || true echo echo "== package-manager probe callsites ==" rg -n "\\$`(pnpm --version|corepack enable pnpm)" cli/src/utils/package-manager.ts cli/src/utils/remote-resolver.ts -n -C 2Repository: flanksource/facet
Length of output: 4263
🏁 Script executed:
# Read package-manager.ts to see how the shell wrapper is used cat -n cli/src/utils/package-manager.tsRepository: flanksource/facet
Length of output: 5401
🏁 Script executed:
# Check for Windows-specific handling and platform detection echo "== Platform detection in codebase ==" rg "process\.platform|win32|windows" cli/src --type ts -i echo echo "== Where resolvePackageManager is called ==" rg "resolvePackageManager" cli/src --type ts -B 2 -A 2 echo echo "== Check if shell.ts has any platform detection ==" rg "process\.|os\.|platform" cli/src/utils/shell.ts --type tsRepository: flanksource/facet
Length of output: 7731
Fix shell wrapper cross-platform compatibility on Windows
The shell module at line 11 uses
/bin/shunconditionally, which does not exist on Windows. This blockspnpm --versionandcorepack enable pnpmcommands, preventing pnpm resolution from starting on Windows. The issue affects critical flows: install operations in vite-builder, remote template resolution, and the doctor diagnostic command all depend onresolvePackageManager()which will fail immediately with ENOENT on Windows systems.🤖 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/package-manager.ts` at line 11, The shell module being imported here uses an unconditional `/bin/sh` path that does not exist on Windows, causing ENOENT errors when executing commands like pnpm version checks. In the shell module being imported, replace the hardcoded `/bin/sh` with cross-platform compatible shell detection that uses `cmd.exe` on Windows systems and `/bin/sh` on Unix-like systems. This will ensure the $ function works correctly across all platforms and fix the broken pnpm resolution flows in vite-builder, template resolution, and doctor commands that depend on resolvePackageManager().cli/test/pdf-security.test.ts (1)
13-13:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect global FACET_PACKAGE_PATH mutations and whether tests restore them. rg -n -C3 "FACET_PACKAGE_PATH|process\\.env\\[" cli/test cli/srcRepository: flanksource/facet
Length of output: 20900
🏁 Script executed:
# Read the full pdf-security.test.ts file to check for afterAll cleanup wc -l cli/test/pdf-security.test.tsRepository: flanksource/facet
Length of output: 95
🏁 Script executed:
# Read the full render-api.test.ts file to check for afterAll cleanup wc -l cli/test/render-api.test.tsRepository: flanksource/facet
Length of output: 93
🏁 Script executed:
cat -n cli/test/pdf-security.test.tsRepository: flanksource/facet
Length of output: 9626
🏁 Script executed:
cat -n cli/test/render-api.test.ts | head -60Repository: flanksource/facet
Length of output: 2423
Save and restore
FACET_PACKAGE_PATHin module cleanup to prevent test leakage.Line 13 sets
FACET_PACKAGE_PATHat module scope without restoration, which pollutes the environment for tests running in the same worker process. Store the previous value and restore it inafterAll:const savedPath = process.env['FACET_PACKAGE_PATH']; process.env['FACET_PACKAGE_PATH'] = REPO_ROOT; // In afterAll: if (savedPath === undefined) delete process.env['FACET_PACKAGE_PATH']; else process.env['FACET_PACKAGE_PATH'] = savedPath;The same issue exists in
cli/test/render-api.test.tsat line 21.🤖 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/test/pdf-security.test.ts` at line 13, The assignment of process.env['FACET_PACKAGE_PATH'] at module scope in cli/test/pdf-security.test.ts at line 13 and cli/test/render-api.test.ts at line 21 causes environment pollution for subsequent tests in the same worker process. For each file, save the previous value of process.env['FACET_PACKAGE_PATH'] before the assignment (store it in a const like savedPath), and add or update an afterAll block to restore the original state by either deleting the environment variable if it was previously undefined, or restoring the saved value if it existed.
- shell.ts: array interpolation escapes each element as a separate arg; signal-terminated children now reject with signal details - vite-builder/vite-ssr-loader: shell-safe tsx argv; NODE_PATH uses delimiter - facet-directory: .npmrc written 0o600 (may carry auth tokens) - doctor: checkTsx probes the runtime resolver, not bare PATH - preview: stream loop honors res.write backpressure - Badge: nullish label guard so 0/"" render; ListTable: preserve passthrough classes; Arrow: showTail on bidirectional; content: unique React keys - styles.css @import string notation; HeaderSolid title 14mm; CI/docs cleanup Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/src/utils/tailwind.test.ts (2)
49-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the fixture binary name platform-aware.
Lines 52–54 only create
tailwindcss, but Windows resolution usestailwindcss.cmd. This test can fail onwin32.Suggested fix
const binDir = join(facetRoot, 'node_modules', '.bin'); mkdirSync(binDir, { recursive: true }); - writeFileSync(join(binDir, 'tailwindcss'), '#!/bin/sh\n'); - chmodSync(join(binDir, 'tailwindcss'), 0o755); + const binName = process.platform === 'win32' ? 'tailwindcss.cmd' : 'tailwindcss'; + writeFileSync(join(binDir, binName), '#!/bin/sh\n'); + if (process.platform !== 'win32') chmodSync(join(binDir, binName), 0o755); expect(tailwindBinExists(facetRoot)).toBe(true);🤖 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/tailwind.test.ts` around lines 49 - 54, The test fixture in the tailwindBinExists test only creates the tailwindcss binary file, but on Windows systems, the binary resolution uses tailwindcss.cmd. Make the binary name platform-aware by detecting the current operating system and creating the appropriate binary file name: use tailwindcss.cmd on win32 platforms and tailwindcss on other platforms. This ensures the test passes consistently across both Unix-like systems and Windows.
19-30:⚠️ Potential issue | 🟠 MajorReplace
Object.definePropertymocking with dependency injection to avoid global state pollution during parallel test execution.The current approach of using
Object.definePropertyto mockprocess.platformis inherently unstable. While the code includes anafterEachrestoration hook, this pattern relies on global mutation that can cause race conditions and unpredictable behavior when tests run in parallel (which Bun does by default). Additionally, mockingprocess.platformdoes not simulate the actual environment—underlying OS-specific behaviors remain unchanged.Refactor
resolveTailwindBin()to accept an optionalplatformparameter or create a wrapper function that abstracts platform detection, allowing tests to inject the desired value without mutating global state:export function resolveTailwindBin(facetRoot: string, platform: string = process.platform): string { const name = platform === 'win32' ? 'tailwindcss.cmd' : 'tailwindcss'; return join(facetRoot, 'node_modules', '.bin', name); }Then tests can simply call
resolveTailwindBin(path, 'linux')orresolveTailwindBin(path, 'win32')without modifying global state.🤖 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/tailwind.test.ts` around lines 19 - 30, The resolveTailwindBin function currently relies on global process.platform, which causes tests to mutate global state through Object.defineProperty, leading to race conditions during parallel execution. Refactor resolveTailwindBin to accept an optional platform parameter (defaulting to process.platform if not provided) and use that parameter to determine whether to return the windows or POSIX path. Then update the test cases to pass the platform value directly as a second argument to resolveTailwindBin (e.g., resolveTailwindBin('/tmp/.facet', 'linux') and resolveTailwindBin('C:\\proj\\.facet', 'win32')) and remove the Object.defineProperty mocking calls to eliminate global state pollution.cli/package.json (1)
8-10:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
binpoints to an artifact this build does not produce.Line 9 maps the executable to
./dist/cli.mjs, but Line 19 writes../dist/facet, andcli/tsdown.config.tsLine 5 only buildssrc/index.ts. This will make the published CLI entrypoint missing or stale.Also applies to: 19-23
🤖 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/package.json` around lines 8 - 10, The bin field in package.json points to ./dist/cli.mjs as the CLI executable entry point, but this does not match the actual build output path or what is configured in the build tool. Review the tsdown.config.ts file to determine what file is actually being built and where the build output is placed (referenced on line 19 as ../dist/facet), then update the bin entry in package.json to point to the correct built artifact path and filename that the build system actually produces. Ensure all references are consistent across package.json, the build configuration, and any build output paths.
🤖 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/package.json`:
- Line 2: The package name field in the cli package.json file is set to "facet"
but the release target expects the scoped package name "`@flanksource/facet-cli`".
Update the "name" property to use the correct scoped package name
"`@flanksource/facet-cli`" to ensure the release automation publishes the correct
package and consumers can access it with the expected name.
- Around line 18-26: The build and test scripts in the package.json
(specifically `build:cli`, `test`, and `test:watch`) are hardcoded to use Bun as
the only runtime, which creates friction in environments where Bun is not
installed. To address this, either provide alternative scripts using Node/npm
that are compatible with non-Bun environments, or add clear documentation (such
as a README section or inline comment in package.json) explicitly stating that
Bun is a required dependency for building and testing this project. Given that
the codebase already contains Bun-specific APIs like `import { $ } from 'bun'`
and `Bun.spawn`, documenting the requirement is the recommended approach.
In `@cli/src/builders/facet-directory.test.ts`:
- Line 1: The import statement in the facet-directory.test.ts file is importing
test utilities from 'bun:test' but the PR objective requires migration to
Vitest. Replace the import statement that currently imports describe, it,
expect, beforeEach, and afterEach from 'bun:test' with an equivalent import from
'vitest' instead. The imported functions remain the same, only the import source
needs to be changed.
In `@cli/src/commands/doctor.ts`:
- Line 10: The doctor command currently uses Bun-specific runtime APIs that are
incompatible with Node.js. Remove the import statement `import { $ } from 'bun'`
from the top of the file and replace the `Bun.spawnSync` call on line 229 with
the Node.js equivalent from the child_process module (use `execSync` or
`spawnSync` from child_process). Update the method call and any related
parameters to match Node.js child_process API conventions to ensure the doctor
command works correctly under both Node.js and Bun runtimes.
- Line 20: The import statement for rootPackageJson uses Bun-specific syntax
with the `type: 'file'` attribute which is not supported in Node.js. Replace the
import statement with a Node.js-compatible approach by using fileURLToPath and
import.meta.url to create a file path string. Create a constant using new URL()
to resolve the relative path '../../../package.json' against import.meta.url,
then pass it to fileURLToPath() to get an absolute path that can be used with
readFileSync() at line 214 where rootPackageJson is consumed.
In `@cli/src/utils/package-manager.ts`:
- Line 11: The code currently imports the Bun-specific `$` utility from 'bun' on
line 11 of package-manager.ts and line 6 of remote-resolver.ts, making these
modules incompatible with Node.js runtime despite the CLI's Node.js shebang and
engine requirements. Create a runtime-agnostic shell abstraction module that
conditionally uses Bun's `$` when running under Bun or Node.js's `child_process`
module when running under Node.js, then update the imports in both
package-manager.ts and remote-resolver.ts to use this abstraction instead of
directly importing from 'bun'. Apply the abstraction to all shell command
executions within the functions tryProbePnpmVersion(), tryCorepackEnablePnpm(),
resolvePackageManager(), and any other functions that invoke shell commands
through the `$` utility.
In `@cli/tsdown.config.ts`:
- Line 12: The library targets Node.js but exports startServer and createServer
that exclusively use Bun.serve() with no Node.js fallback. This creates a
fundamental design mismatch. You need to resolve this by choosing one of three
approaches: (1) implement Node.js-compatible versions of startServer and
createServer using the http module alongside the Bun implementations, (2) remove
these functions from the public exports and keep them as internal utilities for
CLI use only, or (3) change the build configuration to target Bun explicitly
instead of Node.js. Simply having 'bun' in the external array does not resolve
the core issue since bun is not listed as a dependency and the bundler cannot
resolve it anyway.
---
Outside diff comments:
In `@cli/package.json`:
- Around line 8-10: The bin field in package.json points to ./dist/cli.mjs as
the CLI executable entry point, but this does not match the actual build output
path or what is configured in the build tool. Review the tsdown.config.ts file
to determine what file is actually being built and where the build output is
placed (referenced on line 19 as ../dist/facet), then update the bin entry in
package.json to point to the correct built artifact path and filename that the
build system actually produces. Ensure all references are consistent across
package.json, the build configuration, and any build output paths.
In `@cli/src/utils/tailwind.test.ts`:
- Around line 49-54: The test fixture in the tailwindBinExists test only creates
the tailwindcss binary file, but on Windows systems, the binary resolution uses
tailwindcss.cmd. Make the binary name platform-aware by detecting the current
operating system and creating the appropriate binary file name: use
tailwindcss.cmd on win32 platforms and tailwindcss on other platforms. This
ensures the test passes consistently across both Unix-like systems and Windows.
- Around line 19-30: The resolveTailwindBin function currently relies on global
process.platform, which causes tests to mutate global state through
Object.defineProperty, leading to race conditions during parallel execution.
Refactor resolveTailwindBin to accept an optional platform parameter (defaulting
to process.platform if not provided) and use that parameter to determine whether
to return the windows or POSIX path. Then update the test cases to pass the
platform value directly as a second argument to resolveTailwindBin (e.g.,
resolveTailwindBin('/tmp/.facet', 'linux') and
resolveTailwindBin('C:\\proj\\.facet', 'win32')) and remove the
Object.defineProperty mocking calls to eliminate global state pollution.
🪄 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: a1b45e5c-3ae1-491c-afd6-5606f9696b21
⛔ Files ignored due to path filters (1)
cli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.github/workflows/release.ymlcli/README.mdcli/package.jsoncli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/commands/doctor.test.tscli/src/commands/doctor.tscli/src/server/preview.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/tailwind.test.tscli/src/utils/tailwind.tscli/tsdown.config.tscli/vite-dev-loader.tsexamples/kitchen-sink/HeaderSolid.tsxsrc/components/Badge.tsxsrc/components/ListTable.tsxsrc/components/diagram/Arrow.tsxsrc/components/diagram/content.tsxsrc/styles.css
💤 Files with no reviewable changes (1)
- cli/README.md
✅ Files skipped from review due to trivial changes (4)
- examples/kitchen-sink/HeaderSolid.tsx
- src/styles.css
- cli/src/bundler/vite-builder.ts
- cli/src/server/preview.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/diagram/content.tsx
- cli/src/utils/tailwind.ts
- src/components/diagram/Arrow.tsx
- cli/vite-dev-loader.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cli/src/utils/tailwind.test.ts (2)
49-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the fixture binary name platform-aware.
Lines 52–54 only create
tailwindcss, but Windows resolution usestailwindcss.cmd. This test can fail onwin32.Suggested fix
const binDir = join(facetRoot, 'node_modules', '.bin'); mkdirSync(binDir, { recursive: true }); - writeFileSync(join(binDir, 'tailwindcss'), '#!/bin/sh\n'); - chmodSync(join(binDir, 'tailwindcss'), 0o755); + const binName = process.platform === 'win32' ? 'tailwindcss.cmd' : 'tailwindcss'; + writeFileSync(join(binDir, binName), '#!/bin/sh\n'); + if (process.platform !== 'win32') chmodSync(join(binDir, binName), 0o755); expect(tailwindBinExists(facetRoot)).toBe(true);🤖 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/tailwind.test.ts` around lines 49 - 54, The test fixture in the tailwindBinExists test only creates the tailwindcss binary file, but on Windows systems, the binary resolution uses tailwindcss.cmd. Make the binary name platform-aware by detecting the current operating system and creating the appropriate binary file name: use tailwindcss.cmd on win32 platforms and tailwindcss on other platforms. This ensures the test passes consistently across both Unix-like systems and Windows.
19-30:⚠️ Potential issue | 🟠 MajorReplace
Object.definePropertymocking with dependency injection to avoid global state pollution during parallel test execution.The current approach of using
Object.definePropertyto mockprocess.platformis inherently unstable. While the code includes anafterEachrestoration hook, this pattern relies on global mutation that can cause race conditions and unpredictable behavior when tests run in parallel (which Bun does by default). Additionally, mockingprocess.platformdoes not simulate the actual environment—underlying OS-specific behaviors remain unchanged.Refactor
resolveTailwindBin()to accept an optionalplatformparameter or create a wrapper function that abstracts platform detection, allowing tests to inject the desired value without mutating global state:export function resolveTailwindBin(facetRoot: string, platform: string = process.platform): string { const name = platform === 'win32' ? 'tailwindcss.cmd' : 'tailwindcss'; return join(facetRoot, 'node_modules', '.bin', name); }Then tests can simply call
resolveTailwindBin(path, 'linux')orresolveTailwindBin(path, 'win32')without modifying global state.🤖 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/tailwind.test.ts` around lines 19 - 30, The resolveTailwindBin function currently relies on global process.platform, which causes tests to mutate global state through Object.defineProperty, leading to race conditions during parallel execution. Refactor resolveTailwindBin to accept an optional platform parameter (defaulting to process.platform if not provided) and use that parameter to determine whether to return the windows or POSIX path. Then update the test cases to pass the platform value directly as a second argument to resolveTailwindBin (e.g., resolveTailwindBin('/tmp/.facet', 'linux') and resolveTailwindBin('C:\\proj\\.facet', 'win32')) and remove the Object.defineProperty mocking calls to eliminate global state pollution.cli/package.json (1)
8-10:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
binpoints to an artifact this build does not produce.Line 9 maps the executable to
./dist/cli.mjs, but Line 19 writes../dist/facet, andcli/tsdown.config.tsLine 5 only buildssrc/index.ts. This will make the published CLI entrypoint missing or stale.Also applies to: 19-23
🤖 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/package.json` around lines 8 - 10, The bin field in package.json points to ./dist/cli.mjs as the CLI executable entry point, but this does not match the actual build output path or what is configured in the build tool. Review the tsdown.config.ts file to determine what file is actually being built and where the build output is placed (referenced on line 19 as ../dist/facet), then update the bin entry in package.json to point to the correct built artifact path and filename that the build system actually produces. Ensure all references are consistent across package.json, the build configuration, and any build output paths.
🤖 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/package.json`:
- Line 2: The package name field in the cli package.json file is set to "facet"
but the release target expects the scoped package name "`@flanksource/facet-cli`".
Update the "name" property to use the correct scoped package name
"`@flanksource/facet-cli`" to ensure the release automation publishes the correct
package and consumers can access it with the expected name.
- Around line 18-26: The build and test scripts in the package.json
(specifically `build:cli`, `test`, and `test:watch`) are hardcoded to use Bun as
the only runtime, which creates friction in environments where Bun is not
installed. To address this, either provide alternative scripts using Node/npm
that are compatible with non-Bun environments, or add clear documentation (such
as a README section or inline comment in package.json) explicitly stating that
Bun is a required dependency for building and testing this project. Given that
the codebase already contains Bun-specific APIs like `import { $ } from 'bun'`
and `Bun.spawn`, documenting the requirement is the recommended approach.
In `@cli/src/builders/facet-directory.test.ts`:
- Line 1: The import statement in the facet-directory.test.ts file is importing
test utilities from 'bun:test' but the PR objective requires migration to
Vitest. Replace the import statement that currently imports describe, it,
expect, beforeEach, and afterEach from 'bun:test' with an equivalent import from
'vitest' instead. The imported functions remain the same, only the import source
needs to be changed.
In `@cli/src/commands/doctor.ts`:
- Line 10: The doctor command currently uses Bun-specific runtime APIs that are
incompatible with Node.js. Remove the import statement `import { $ } from 'bun'`
from the top of the file and replace the `Bun.spawnSync` call on line 229 with
the Node.js equivalent from the child_process module (use `execSync` or
`spawnSync` from child_process). Update the method call and any related
parameters to match Node.js child_process API conventions to ensure the doctor
command works correctly under both Node.js and Bun runtimes.
- Line 20: The import statement for rootPackageJson uses Bun-specific syntax
with the `type: 'file'` attribute which is not supported in Node.js. Replace the
import statement with a Node.js-compatible approach by using fileURLToPath and
import.meta.url to create a file path string. Create a constant using new URL()
to resolve the relative path '../../../package.json' against import.meta.url,
then pass it to fileURLToPath() to get an absolute path that can be used with
readFileSync() at line 214 where rootPackageJson is consumed.
In `@cli/src/utils/package-manager.ts`:
- Line 11: The code currently imports the Bun-specific `$` utility from 'bun' on
line 11 of package-manager.ts and line 6 of remote-resolver.ts, making these
modules incompatible with Node.js runtime despite the CLI's Node.js shebang and
engine requirements. Create a runtime-agnostic shell abstraction module that
conditionally uses Bun's `$` when running under Bun or Node.js's `child_process`
module when running under Node.js, then update the imports in both
package-manager.ts and remote-resolver.ts to use this abstraction instead of
directly importing from 'bun'. Apply the abstraction to all shell command
executions within the functions tryProbePnpmVersion(), tryCorepackEnablePnpm(),
resolvePackageManager(), and any other functions that invoke shell commands
through the `$` utility.
In `@cli/tsdown.config.ts`:
- Line 12: The library targets Node.js but exports startServer and createServer
that exclusively use Bun.serve() with no Node.js fallback. This creates a
fundamental design mismatch. You need to resolve this by choosing one of three
approaches: (1) implement Node.js-compatible versions of startServer and
createServer using the http module alongside the Bun implementations, (2) remove
these functions from the public exports and keep them as internal utilities for
CLI use only, or (3) change the build configuration to target Bun explicitly
instead of Node.js. Simply having 'bun' in the external array does not resolve
the core issue since bun is not listed as a dependency and the bundler cannot
resolve it anyway.
---
Outside diff comments:
In `@cli/package.json`:
- Around line 8-10: The bin field in package.json points to ./dist/cli.mjs as
the CLI executable entry point, but this does not match the actual build output
path or what is configured in the build tool. Review the tsdown.config.ts file
to determine what file is actually being built and where the build output is
placed (referenced on line 19 as ../dist/facet), then update the bin entry in
package.json to point to the correct built artifact path and filename that the
build system actually produces. Ensure all references are consistent across
package.json, the build configuration, and any build output paths.
In `@cli/src/utils/tailwind.test.ts`:
- Around line 49-54: The test fixture in the tailwindBinExists test only creates
the tailwindcss binary file, but on Windows systems, the binary resolution uses
tailwindcss.cmd. Make the binary name platform-aware by detecting the current
operating system and creating the appropriate binary file name: use
tailwindcss.cmd on win32 platforms and tailwindcss on other platforms. This
ensures the test passes consistently across both Unix-like systems and Windows.
- Around line 19-30: The resolveTailwindBin function currently relies on global
process.platform, which causes tests to mutate global state through
Object.defineProperty, leading to race conditions during parallel execution.
Refactor resolveTailwindBin to accept an optional platform parameter (defaulting
to process.platform if not provided) and use that parameter to determine whether
to return the windows or POSIX path. Then update the test cases to pass the
platform value directly as a second argument to resolveTailwindBin (e.g.,
resolveTailwindBin('/tmp/.facet', 'linux') and
resolveTailwindBin('C:\\proj\\.facet', 'win32')) and remove the
Object.defineProperty mocking calls to eliminate global state pollution.
🪄 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: a1b45e5c-3ae1-491c-afd6-5606f9696b21
⛔ Files ignored due to path filters (1)
cli/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (22)
.github/workflows/release.ymlcli/README.mdcli/package.jsoncli/src/builders/facet-directory.test.tscli/src/builders/facet-directory.tscli/src/bundler/vite-builder.tscli/src/bundler/vite-server.tscli/src/commands/doctor.test.tscli/src/commands/doctor.tscli/src/server/preview.tscli/src/utils/package-manager.test.tscli/src/utils/package-manager.tscli/src/utils/tailwind.test.tscli/src/utils/tailwind.tscli/tsdown.config.tscli/vite-dev-loader.tsexamples/kitchen-sink/HeaderSolid.tsxsrc/components/Badge.tsxsrc/components/ListTable.tsxsrc/components/diagram/Arrow.tsxsrc/components/diagram/content.tsxsrc/styles.css
💤 Files with no reviewable changes (1)
- cli/README.md
✅ Files skipped from review due to trivial changes (4)
- examples/kitchen-sink/HeaderSolid.tsx
- src/styles.css
- cli/src/bundler/vite-builder.ts
- cli/src/server/preview.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/diagram/content.tsx
- cli/src/utils/tailwind.ts
- src/components/diagram/Arrow.tsx
- cli/vite-dev-loader.ts
🛑 Comments failed to post (7)
cli/package.json (2)
2-2:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winPackage name diverges from the release target.
Line 2 sets
"name": "facet", but the release objective specifies publishing@flanksource/facet-cli. This can publish the wrong package and break release automation/consumers expecting the scoped name.🤖 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/package.json` at line 2, The package name field in the cli package.json file is set to "facet" but the release target expects the scoped package name "`@flanksource/facet-cli`". Update the "name" property to use the correct scoped package name "`@flanksource/facet-cli`" to ensure the release automation publishes the correct package and consumers can access it with the expected name.
18-26:
⚠️ Potential issue | 🟠 Major🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Verify Bun is now required by package scripts rg -n '"(build:cli|test|test:watch)"\s*:\s*"[^"]*\bbun\b' cli/package.json # Verify Bun API usage still exists in source/test trees rg -nP '\bfrom\s+[\"\x27]bun[\"\x27]|\brequire\([\"\x27]bun[\"\x27]\)|\bBun\.' cli/src cli/test cli || trueRepository: flanksource/facet
Length of output: 1978
🏁 Script executed:
# Check line 72 of cli/package.json sed -n '70,75p' cli/package.json # Look for migration goals or Node requirements in package.json cat -n cli/package.json | head -80Repository: flanksource/facet
Length of output: 2979
🏁 Script executed:
# Search for any migration-related comments or goals in the codebase rg -i "node.*migration|migrate.*node|node.*contract" --max-count=20Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Look for README or migration documentation find . -maxdepth 2 -type f \( -name "README*" -o -name "CONTRIBUTING*" -o -name "*.md" \) | head -20 # Check root README for any migration discussion cat README.md 2>/dev/null | head -100 || echo "No root README found" # Check for docs directory ls -la docs/ 2>/dev/null || echo "No docs directory"Repository: flanksource/facet
Length of output: 2796
🏁 Script executed:
# Search for migration-related context in any files rg -i "migrate\s+to\s+node|move\s+to\s+node|node-only|npm-only" --max-count=10Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
cat CONTRIBUTING.md 2>/dev/null | head -50 || echo "File not found or empty"Repository: flanksource/facet
Length of output: 43
Bun is required in build/test scripts despite environment constraints.
Lines 19, 25-26 establish
bun build,bun test, andbun test:watchas the only available commands, while line 72 adds@types/bunas a devDependency. This hard requirement on Bun can create friction in CI systems or contributor environments where Bun is not installed, and the codebase already uses Bun-specific APIs extensively (e.g.,import { $ } from 'bun',Bun.spawn,Bun.serveacross multiple files). Consider providing Node/npm equivalents or documenting the Bun requirement explicitly.🤖 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/package.json` around lines 18 - 26, The build and test scripts in the package.json (specifically `build:cli`, `test`, and `test:watch`) are hardcoded to use Bun as the only runtime, which creates friction in environments where Bun is not installed. To address this, either provide alternative scripts using Node/npm that are compatible with non-Bun environments, or add clear documentation (such as a README section or inline comment in package.json) explicitly stating that Bun is a required dependency for building and testing this project. Given that the codebase already contains Bun-specific APIs like `import { $ } from 'bun'` and `Bun.spawn`, documenting the requirement is the recommended approach.cli/src/builders/facet-directory.test.ts (1)
1-1:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest imports
bun:testbut PR objective states migration to Vitest.The PR summary indicates "migrates
bun:test→vitest", but this file imports frombun:test. This will fail when running tests under Node.js with Vitest.Proposed fix
-import { describe, it, expect, beforeEach, afterEach } from 'bun:test'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest';📝 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 { describe, it, expect, beforeEach, afterEach } from 'vitest';🤖 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/builders/facet-directory.test.ts` at line 1, The import statement in the facet-directory.test.ts file is importing test utilities from 'bun:test' but the PR objective requires migration to Vitest. Replace the import statement that currently imports describe, it, expect, beforeEach, and afterEach from 'bun:test' with an equivalent import from 'vitest' instead. The imported functions remain the same, only the import source needs to be changed.cli/src/commands/doctor.ts (2)
10-10:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify Bun runtime API usage still present in CLI sources. rg -n "from 'bun'|from \"bun\"|\\bBun\\." cli/srcRepository: flanksource/facet
Length of output: 940
Remove Bun runtime APIs from doctor command to ensure Node.js compatibility.
The import on line 10 (
import { $ } from 'bun') and the Bun API call on line 229 (Bun.spawnSync) require the Bun runtime and will cause the doctor command to fail when run under Node.js.Replace with Node.js equivalents:
Suggested fix
-import { $ } from 'bun'; +import { $ } from '../utils/shell.js'; +import { spawnSync } from 'node:child_process'; ... - hostArch = Bun.spawnSync(['uname', '-m']).stdout.toString().trim(); + hostArch = spawnSync('uname', ['-m'], { encoding: 'utf-8' }).stdout.trim();🤖 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/commands/doctor.ts` at line 10, The doctor command currently uses Bun-specific runtime APIs that are incompatible with Node.js. Remove the import statement `import { $ } from 'bun'` from the top of the file and replace the `Bun.spawnSync` call on line 229 with the Node.js equivalent from the child_process module (use `execSync` or `spawnSync` from child_process). Update the method call and any related parameters to match Node.js child_process API conventions to ensure the doctor command works correctly under both Node.js and Bun runtimes.
20-20:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and read the relevant lines if [ -f "cli/src/commands/doctor.ts" ]; then wc -l cli/src/commands/doctor.ts echo "=== Lines 1-30 ===" sed -n '1,30p' cli/src/commands/doctor.ts | cat -n echo "=== Lines 210-220 ===" sed -n '210,220p' cli/src/commands/doctor.ts | cat -n else echo "File not found" fiRepository: flanksource/facet
Length of output: 1708
🌐 Web query:
Node.js import attributes with type file syntax compatibility💡 Result:
In Node.js, the syntax for importing files—specifically JSON modules—has evolved from "import assertions" to the standardized "import attributes" [1][2][3]. Current Syntax (Standardized) The current, standardized syntax uses the
withkeyword [4][5][2]: import data from './data.json' with { type: 'json' }; const { default: barData } = await import('./bar.json', { with: { type: 'json' } }); Compatibility Summary - Node.js v22.0.0 and later: Only thewithsyntax is supported [4][5][2]. Support for the olderassertkeyword was explicitly removed [4][2]. - Node.js v18.20.0 and v20.10.0: Support for thewithsyntax (import attributes) was added [4][1]. - Prior Versions (e.g., v16.x, v17.x, older v18.x): These versions used theassertkeyword (import assertions) [6][3], which is now deprecated and incompatible with modern Node.js environments [5][2]. Key Details - Mandatory Attribute: Thetype: 'json'attribute is required when importing JSON files [4][7][8]. Node.js currently only supports thetypeattribute for this purpose [4][1][3]. - Migration: If you are upgrading a codebase from older Node.js versions, you must replace all instances ofassertwithwith[5][2]. A codemod is available vianpx codemod nodejs/import-assertions-to-attributesto automate this transition [5][9].Citations:
- 1: https://nodejs.org/api/esm.html
- 2: nodejs/node#52104
- 3: https://stackoverflow.com/questions/78876691/syntaxerror-unexpected-identifier-assert-on-json-import-in-node-v22
- 4: https://nodejs.org/docs/latest-v26.x/api/esm.html
- 5: https://nodejs-userland-migrations.mintlify.app/recipes/import-assertions-to-attributes
- 6: https://stackoverflow.com/questions/60205891/import-json-extension-in-es6-node-js-throws-an-error
- 7: https://nodejs.org/docs/latest-v23.x/api/esm.html
- 8: https://nodejs.org/download/release/v22.14.0/docs/api/esm.html
- 9: https://github.com/nodejs/userland-migrations/tree/main/recipes/import-assertions-to-attributes
🌐 Web query:
Bun import attributes type file JSON import syntax💡 Result:
In Bun, you can import JSON files natively using standard ES module syntax [1][2]. While the
.jsonextension is often sufficient for automatic handling [2][3], you can explicitly specify the type using import attributes [1][4]. Current Import Syntax To import a JSON file, use the following syntax [1]: import data from "./data.json"; For explicit typing—which can be useful for clarity or when working with files that do not have a.json extension—you can use thewithkeyword, which follows the Stage 3 ECMA Script proposal for import attributes [1][5]: import data from "./data.json" with { type: "json" }; Dynamic Import Syntax For dynamic imports, use thewithoption [2][6]: const data = await import("./data.json", { with: { type: "json" } }); Key Details - Import Attributes vs. Assertions: Bun supports the modernwithsyntax (Import Attributes) [1][5]. While an older syntax usingassertpreviously existed (Import Assertions), it has been abandoned by the TC39 proposal in favor ofwith[5][7]. - Bundler Compatibility: When bundling with Bun, JSON files imported in this manner are inlined into the resulting JavaScript bundle as objects [2][3]. - Other Supported Types: The samewith { type: "..." }syntax is supported by Bun for other loaders, includingtoml,text, andfile[6][4]. For scenarios where you do not need the file parsed as a module, you can also useBun.file()to read JSON files lazily [8].Citations:
- 1: https://bun.com/docs/guides/runtime/import-json
- 2: https://bun.com/docs/runtime/file-types
- 3: https://bun.com/docs/bundler/loaders
- 4: https://zhcndoc-bun.mintlify.app/runtime/modules
- 5: https://bun.com/docs/bundler/macros
- 6: oven-sh/bun#10456
- 7: https://bun.com/blog/bun-macros
- 8: https://bun.com/docs/guides/read-file/json
with { type: 'file' }is Bun-specific and incompatible with Node.js.Line 20 uses a Bun-only import attribute syntax. Node.js does not support
type: 'file'(it only supportstype: 'json'for import attributes). Since line 214 expectsrootPackageJsonto be a file path forreadFileSync(), convert to a Node.js-compatible path usingfileURLToPathandimport.meta.url:Suggested fix
-import rootPackageJson from '../../../package.json' with { type: 'file' }; +import { fileURLToPath } from 'node:url'; +const rootPackageJson = fileURLToPath(new URL('../../../package.json', import.meta.url));📝 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 { fileURLToPath } from 'node:url'; const rootPackageJson = fileURLToPath(new URL('../../../package.json', import.meta.url));🤖 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/commands/doctor.ts` at line 20, The import statement for rootPackageJson uses Bun-specific syntax with the `type: 'file'` attribute which is not supported in Node.js. Replace the import statement with a Node.js-compatible approach by using fileURLToPath and import.meta.url to create a file path string. Create a constant using new URL() to resolve the relative path '../../../package.json' against import.meta.url, then pass it to fileURLToPath() to get an absolute path that can be used with readFileSync() at line 214 where rootPackageJson is consumed.cli/src/utils/package-manager.ts (1)
11-11:
⚠️ Potential issue | 🔴 Critical🧩 Analysis chain
🏁 Script executed:
# First, check the structure of the cli/src/utils directory ls -la cli/src/utils/ | head -20Repository: flanksource/facet
Length of output: 1329
🏁 Script executed:
# Read the package-manager.ts file to understand the imports and usage cat -n cli/src/utils/package-manager.tsRepository: flanksource/facet
Length of output: 5394
🏁 Script executed:
# Check if shell.js exists in the same directory ls -la cli/src/utils/shell.js 2>&1 || echo "File not found"Repository: flanksource/facet
Length of output: 146
🏁 Script executed:
# Search for resolvePackageManager function to understand its purpose rg "resolvePackageManager" cli/src/utils/package-manager.ts -A 5Repository: flanksource/facet
Length of output: 416
🏁 Script executed:
# Search for shell.js anywhere in the project find . -name "shell.js" -o -name "shell.ts" 2>/dev/nullRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Check if there's a shell wrapper or command execution utility elsewhere rg "export.*\$.*from" --type ts cli/src/utils/ 2>/dev/null || echo "No matches"Repository: flanksource/facet
Length of output: 72
🏁 Script executed:
# Look for shell command utilities in the entire cli/src fd -e ts -e js cli/src | xargs rg "shell|cmd|exec" --type ts 2>/dev/null | head -20Repository: flanksource/facet
Length of output: 2222
🏁 Script executed:
# Check the package.json to understand the runtime target head -30 package.jsonRepository: flanksource/facet
Length of output: 827
🏁 Script executed:
# Look at the test file to understand how this module is used cat -n cli/src/utils/package-manager.test.tsRepository: flanksource/facet
Length of output: 5047
🏁 Script executed:
# Find all imports of resolvePackageManager rg "resolvePackageManager|from.*package-manager" --type ts --type jsRepository: flanksource/facet
Length of output: 1809
🏁 Script executed:
# Check the CLI entry point and its configuration cat -n cli/package.json 2>/dev/null || echo "No cli/package.json"Repository: flanksource/facet
Length of output: 2980
🏁 Script executed:
# Look for any Node-based entry points or CLI scripts that might use this rg "bin\|entry\|main" cli/ --type json 2>/dev/null | head -20Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Check build configuration (tsconfig, vite, esbuild, etc.) ls -la cli/ | grep -E "tsconfig|vite|esbuild|build"Repository: flanksource/facet
Length of output: 340
🏁 Script executed:
# See if there's a shell.ts wrapper elsewhere or if they use a different approach rg "\$\s*from" --type ts -B 2 -A 2 | head -30Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Check remote-resolver.ts to see how resolvePackageManager is used cat -n cli/src/utils/remote-resolver.ts | head -50Repository: flanksource/facet
Length of output: 1816
🏁 Script executed:
# Check if there's any indication this package is meant to work with Node.js directly rg "bun|node" cli/package.json -A 2 -B 2Repository: flanksource/facet
Length of output: 989
🏁 Script executed:
# Look at the CLI entry point to understand the runtime context cat -n cli/src/cli.ts | head -30Repository: flanksource/facet
Length of output: 1484
The Bun import is real but the suggested fix references a non-existent file.
Line 11 does import
$frombun, and this is used intryProbePnpmVersion()andtryCorepackEnablePnpm(). However, the issue is more nuanced:
- The CLI is compiled with
bun build --compile --target=buninto a standalone Bun binary (see cli/package.json), so the distributeddist/facetbinary works correctly.- The CLI entry point (
cli/src/cli.ts) has a Node.js shebang andpackage.jsonspecifiesengines: { node: >=20.19.0 }, suggesting Node.js compatibility intent.- The suggested fix imports from
./shell.js, which does not exist in the codebase.remote-resolver.tshas the same Bun import on line 6, also affectingresolvePackageManager()usage there.The underlying concern about Bun-specific dependencies is valid, but the fix is incomplete. Creating a working shell abstraction that supports both runtimes (or using Node's
child_processconditionally) would be 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/src/utils/package-manager.ts` at line 11, The code currently imports the Bun-specific `$` utility from 'bun' on line 11 of package-manager.ts and line 6 of remote-resolver.ts, making these modules incompatible with Node.js runtime despite the CLI's Node.js shebang and engine requirements. Create a runtime-agnostic shell abstraction module that conditionally uses Bun's `$` when running under Bun or Node.js's `child_process` module when running under Node.js, then update the imports in both package-manager.ts and remote-resolver.ts to use this abstraction instead of directly importing from 'bun'. Apply the abstraction to all shell command executions within the functions tryProbePnpmVersion(), tryCorepackEnablePnpm(), resolvePackageManager(), and any other functions that invoke shell commands through the `$` utility.cli/tsdown.config.ts (1)
12-12: 🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Detect Bun-specific imports/APIs that would leak into a Node runtime rg -nP '\bfrom\s+[\"\x27]bun[\"\x27]|\brequire\([\"\x27]bun[\"\x27]\)|\bBun\.' cli/src cli/test cli || trueRepository: flanksource/facet
Length of output: 1819
🏁 Script executed:
cat -n cli/tsdown.config.tsRepository: flanksource/facet
Length of output: 636
🏁 Script executed:
# Check if there's a package.json that explains the project purpose fd package.json cli -x cat {}Repository: flanksource/facet
Length of output: 2364
🏁 Script executed:
# Check for any build or runtime configuration that indicates Node vs Bun rg -l "bun|node" cli --type json --type ts --type js | head -20Repository: flanksource/facet
Length of output: 603
🏁 Script executed:
cat -n cli/src/index.tsRepository: flanksource/facet
Length of output: 519
🏁 Script executed:
# Also check the directory structure to understand separation fd -t f '\.(ts|tsx)$' cli/src --max-depth 2 | grep -E '^cli/src/[^/]+\.ts$' | head -20Repository: flanksource/facet
Length of output: 110
🏁 Script executed:
cat -n cli/src/server/preview.ts | head -60Repository: flanksource/facet
Length of output: 3309
🏁 Script executed:
# Check if there's any conditional imports or Node alternatives rg -B5 -A10 "Bun\.serve|startServer" cli/src/server/preview.tsRepository: flanksource/facet
Length of output: 1121
🏁 Script executed:
rg -B2 -A15 "export.*startServer|export.*createServer" cli/srcRepository: flanksource/facet
Length of output: 2394
🏁 Script executed:
# Check if there's any documentation about server usage fd -t f '\.(md|txt)' cli | head -10 | xargs grep -l "startServer\|server" 2>/dev/null || trueRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Check the actual exports to see what's documented/intended as public API rg "export" cli/src/index.tsRepository: flanksource/facet
Length of output: 470
🏁 Script executed:
# Check if 'bun' is available as an npm package or just the runtime npm view bun --json 2>/dev/null | head -20 || echo "Checking package.json dependencies..."Repository: flanksource/facet
Length of output: 402
🏁 Script executed:
# Check package.json to see if 'bun' is listed anywhere rg "\"bun\"" cli/package.jsonRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Also check if there's any try-catch or platform detection around Bun usage rg -B5 -A5 "typeof Bun|Bun\?" cli/src/server/preview.tsRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Check if 'bun' is in package.json dependencies (not just devDependencies) cat cli/package.json | grep -A50 "dependencies"Repository: flanksource/facet
Length of output: 1075
🏁 Script executed:
# Look at how startServer is actually used - maybe it's only for Bun runtime rg "startServer|createServer" cli/src/cli.ts -B2 -A5Repository: flanksource/facet
Length of output: 384
🏁 Script executed:
# Check the README or docs to see if startServer is documented as public API fd -t f '\.(md|rst)' cliRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Check what the CLI actually imports - is it using startServer from the library or locally? rg "import.*startServer" cli/src/cli.tsRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Let's see what gets exported in the compiled output - check if there's a dist ls -la cli/dist/ 2>/dev/null | head -20 || echo "No dist directory"Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Most important: check if this library is actually meant to be consumed by Node.js users # Look at the main entry point in package.json grep -A2 -B2 '"main"\|"type"' cli/package.jsonRepository: flanksource/facet
Length of output: 441
🏁 Script executed:
# Check if startServer is documented/promoted as a public API in any README find cli -name "README*" -type f -exec cat {} \; 2>/dev/null | head -100Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Final check: does the library build actually include startServer or is it tree-shaken? # Look at what tsdown produces - check for any bundled output or source maps find cli -path "*/dist/*" -name "*.mjs" -o -name "*.d.ts" | head -10Repository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Let's verify if bun module/types are actually available at build time # The key question: if tsdown tries to resolve 'bun' as a module, would it fail? rg "from.*['\"]bun['\"]|import.*bun" cli/src/server/preview.tsRepository: flanksource/facet
Length of output: 43
🏁 Script executed:
# Final critical check: Does preview.ts have any fallback or Node.js server implementation? cat -n cli/src/server/preview.ts | tail -80Repository: flanksource/facet
Length of output: 3450
🏁 Script executed:
# Check if bun namespace is globally available without import, or if there's a type definition setup rg "declare.*Bun|global.*Bun" cli/srcRepository: flanksource/facet
Length of output: 43
Reconsider the architectural approach for
startServerexport in a Node-targeted library.The library publicly exports
startServer(and implicitlycreateServer) which exclusively usesBun.serve()with no fallback. Since this bundle targets Node.js (platform: 'node',target: 'node18'), exporting Bun-only APIs is fundamentally misaligned.Removing
'bun'fromexternalwon't succeed either—bunis not in the package dependencies, so the bundler will fail to resolve it. The real issue is that these server functions should either:
- Have a Node.js implementation (e.g., using
httpmodule) alongside or instead ofBun.serve()- Be removed from public exports and marked internal to CLI only
- Have the library target Bun explicitly instead of Node.js
The current configuration allows accidental imports but doesn't address the core design mismatch.
🤖 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/tsdown.config.ts` at line 12, The library targets Node.js but exports startServer and createServer that exclusively use Bun.serve() with no Node.js fallback. This creates a fundamental design mismatch. You need to resolve this by choosing one of three approaches: (1) implement Node.js-compatible versions of startServer and createServer using the http module alongside the Bun implementations, (2) remove these functions from the public exports and keep them as internal utilities for CLI use only, or (3) change the build configuration to target Bun explicitly instead of Node.js. Simply having 'bun' in the external array does not resolve the core issue since bun is not listed as a dependency and the bundler cannot resolve it anyway.
Member
|
@coderabbitai review |
✅ Action performedReview finished.
|
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
4 participants
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
Ports the facet CLI off the Bun runtime so it can ship as a real, Node-installable npm package (`@flanksource/facet-cli`). Previously the CLI relied on Bun APIs (`Bun.serve`, Bun shell `$`, `Bun.spawn`, `import ... with { type: 'file' }`, `bun:test`) and was distributed as a bun-compiled standalone binary, which made it impossible to `npm install -g`.
Changes
Verification
`node dist/cli.mjs pdf SimpleReport.tsx --data simple-data.json` produces a valid 52KB PDF-1.7 with no Bun runtime present. `vitest run`: 183 passed; the 2 sandbox-only failures are macOS sandbox blocking Chrome (pass when Chrome can launch).
Summary by CodeRabbit
Release Notes
--livemode for HTML/PDF rendering using a live browser environment snapshotfacet doctorwith--json/--fix/--verbose, including more automated remediation and environment warningsserve --cache-dirto choose the render cache locationFACET_PACKAGE_PATHrowHeightandlabelPaddingfacet doctordocumentation and improved installation guidancepuppeteer-coreand addedreact-xarrows