refactor(bundler): revert Bun.build, restore tsdown with factory API#144
Merged
zrosenbauer merged 12 commits intomainfrom Apr 2, 2026
Merged
refactor(bundler): revert Bun.build, restore tsdown with factory API#144zrosenbauer merged 12 commits intomainfrom
zrosenbauer merged 12 commits intomainfrom
Conversation
…red utils
- Revert Bun.build migration — restore tsdown as the bundler
- Factory pattern: `createBundler({ config, cwd, ...lifecycle })`
- Lifecycle hooks: onStart, onFinish, onStepStart, onStepFinish
- Thin public API: `createBundler` + 4 types
- Create `@kidd-cli/utils/node` with `fs.*` and `process.*` wrappers
- Create `@kidd-cli/config/utils` subpath for internal exports
- Single source of truth: `compileTargets` array with target/label/default
- Binary name derived from config → package.json name → 'cli' fallback
- Manifest read once in factory, version + binary name flow through config
- `ResultAsync` replaces `AsyncResult` across all packages
- Remove bun-runner.ts, bun-config.ts, plugins.ts, read-version.ts
Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 49b3a0b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
- Move clean.ts from build/ to utils/ - Remove isCompiledBinary — was a bad heuristic matching any extensionless file - Clean now computes exact binary filenames from resolved compile name + targets - Remove isBuildArtifact export (private helper) - Remove CleanResult export (private type) Co-Authored-By: Claude <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughReplaces Bun-based bundler internals with tsdown/Rolldown: removes bun-config, bun-runner, Bun plugins, and Bun-specific build/watch/compile flows; adds tsdown build/watch config generators, Rolldown-style autoload plugin (transform/resolveId/load), a createBundler factory with lifecycle hooks, async clean/resolveBuildEntry utilities, and many test updates. Moves filesystem/process helpers to Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/lib/detect.ts (1)
87-99:⚠️ Potential issue | 🟠 MajorValidate the parsed
package.jsonbefore casting.
jsonParse()only proves the file is syntactically valid JSON. Values likenullor{ "dependencies": "x" }still pass through Line 99, anddetectProject()will later crash onpkg.dependencies/'@kidd-cli/core' in depsinstead of returning aread_error. Reject non-object payloads here.Suggested fix
+import { z } from 'zod' + import { ok } from '@kidd-cli/utils/fp' import type { ResultAsync } from '@kidd-cli/utils/fp' import { fs } from '@kidd-cli/utils/node' import { jsonParse } from '@kidd-cli/utils/json' @@ interface PackageJson { readonly dependencies?: Record<string, string> readonly devDependencies?: Record<string, string> } + +const packageJsonSchema = z.object({ + dependencies: z.record(z.string(), z.string()).optional(), + devDependencies: z.record(z.string(), z.string()).optional(), +}) @@ const [parseError, data] = jsonParse(content) if (parseError) { return [ { message: `Failed to parse package.json: ${parseError.message}`, @@ null, ] } - return ok(data as PackageJson) + const validated = packageJsonSchema.safeParse(data) + if (!validated.success) { + return [ + { + message: `Failed to parse package.json: ${validated.error.message}`, + path: filePath, + type: 'read_error' as const, + }, + null, + ] + } + + return ok(validated.data) }As per coding guidelines, "Always use Zod for validation at all boundaries (config, CLI args, external data)" from contributing/standards/typescript/coding-style.md.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/lib/detect.ts` around lines 87 - 99, The parsed JSON from jsonParse (variables parseError and data) must be validated with Zod before casting to PackageJson; update the block in detect.ts to run a Zod schema (or z.object({...}) for the PackageJson shape) against data and if validation fails return the same read_error shape (include validation errors in the message and filePath) instead of blindly returning ok(data as PackageJson); ensure you reference jsonParse, parseError, data, ok(), and PackageJson when implementing the validation and error-return path.packages/bundler/src/compile/compile.ts (1)
109-124:⚠️ Potential issue | 🟠 MajorAlways emit
onStepFinishfor a started target.A failed
compileSingleTarget()currently skipsonStepFinish. That breaks the new start/finish lifecycle contract and leaves status/task consumers with an open target step whenever a compile target fails.Suggested fix
- if (result[0] === null && params.lifecycle.onStepFinish) { + if (params.lifecycle.onStepFinish) { await params.lifecycle.onStepFinish({ phase: 'compile', step: 'target', meta }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bundler/src/compile/compile.ts` around lines 109 - 124, The code calls params.lifecycle.onStepFinish only when compileSingleTarget returns success, leaving the lifecycle open on failures; fix by ensuring params.lifecycle.onStepFinish is always invoked for a started target: wrap the compileSingleTarget call in a try/finally (or ensure a finally block) and in the finally call params.lifecycle.onStepFinish({ phase: 'compile', step: 'target', meta }) if params.lifecycle.onStepFinish is defined (keeping existing onStepStart handling), so compileSingleTarget, params.lifecycle.onStepStart, and params.lifecycle.onStepFinish are used as the points to locate changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bundler/src/autoloader/autoload-plugin.ts`:
- Around line 41-51: The load() hook currently regenerates the virtual module
for RESOLVED_VIRTUAL_ID using scanCommandsDir(params.commandsDir) but doesn't
tell Rollup to watch the commands directory; call the plugin context's
addWatchFile(params.commandsDir) when id === RESOLVED_VIRTUAL_ID (inside
load()), before scanning/generating, so changes in params.commandsDir trigger
rebuilds; ensure you reference the same PluginContext used by the plugin (where
load is defined) so addWatchFile is available, then continue to call
scanCommandsDir(...) and generateStaticAutoloader(...) as before.
In `@packages/bundler/src/build/build.ts`:
- Around line 23-25: The cleanup step is performed outside the
AsyncBundlerResult flow and may fail silently — modify the build function so
that the call to clean({ compile: params.compile, outDir:
params.resolved.buildOutDir }) is executed and its failure is handled using the
project's Result/ResultAsync pattern (not throw), short-circuiting the function
and returning a failure AsyncBundlerResult if cleanup fails; only call
tsdownBuild() (and continue the normal success flow) when the clean Result
indicates success. Ensure you update the code paths around
params.resolved.build.clean, clean(), and tsdownBuild() to propagate the Result
type instead of throwing so callers receive a consistent AsyncBundlerResult on
cleanup errors.
In `@packages/bundler/src/build/clean.ts`:
- Around line 69-71: The current code awaits fs.remove inside the isArtifact
branch and unconditionally returns { type: 'removed', name } which can misreport
failures; wrap the await fs.remove(join(params.outDir, name)) in a try/catch,
only return { type: 'removed', name } on success, and on failure return or
propagate a failure payload (e.g., { type: 'error', name, error }) or rethrow
the caught error so callers can detect the failure; reference the isArtifact
branch, fs.remove call, join(params.outDir, name) and the existing return shape
to locate where to add the try/catch (or alternatively add a clear comment if
best-effort cleanup is intended).
In `@packages/bundler/src/build/config.test.ts`:
- Around line 27-108: Add a new test that calls toTsdownBuildConfig({ config,
version: undefined, compile: true }) and asserts the compile branch: check the
returned object’s deps.alwaysBundle contains the expected compile-time externals
(e.g., includes 'pg' and NODE_BUILTINS items) and that the returned plugins
array contains the compile stub plugin (assert presence in result.plugins or by
plugin.name). Use the existing test suite helpers and the toTsdownBuildConfig,
deps, alwaysBundle and plugins symbols to locate and add this assertion.
In `@packages/bundler/src/compile/compile.ts`:
- Around line 225-233: The match branch for 'linux-x64-musl' in the target
mapping inside compile.ts incorrectly returns 'bun-linux-x64' (glibc); update
the mapping in the function that builds compile targets (the match(...)
expression) so the 'linux-x64-musl' case returns 'bun-linux-x64-musl' instead,
leaving all other branches unchanged.
In `@packages/bundler/src/types.ts`:
- Around line 83-87: The lifecycle hooks on BundlerLifecycle (onStart, onFinish,
onStepStart, onStepFinish) must not be allowed to throw and bypass
AsyncBundlerResult; either change their signatures to return Result<void, E> |
ResultAsync<void, E> (or Promise<Result<void,E>>) or, if keeping current
signatures, update every call site (e.g., in createBundler, compile, and
build/watch invocation paths) to wrap calls to these hooks in try/catch and
convert any thrown/rejected error into the AsyncBundlerResult tuple ([error,
null]) so hook failures are folded into the Result path rather than rejecting
the promise.
In `@packages/bundler/src/utils/resolve-build-entry.ts`:
- Around line 27-32: Replace the two-branch if/return with a ts-pattern match:
import match from "ts-pattern" and use match(found).with({ found: true }, () =>
found.path).otherwise(() => undefined) (or equivalent pattern) to return the
path or undefined; update the code that currently uses results.find((r) =>
r.found) and the following if (found) return found.path to use this pattern
match instead (target the variable found and the function resolve-build-entry).
In `@packages/cli/src/commands/build.ts`:
- Around line 45-47: The ternary used to set mergedConfig should be rewritten
using an explicit if/else or ts-pattern match to satisfy the no-ternary rule:
locate the assignment to mergedConfig (currently using shouldCompile ?
mergeCompileTargets({ config, targets: ctx.args.targets }) : config) and replace
it with an if (shouldCompile) { mergedConfig = mergeCompileTargets({ config,
targets: ctx.args.targets }) } else { mergedConfig = config } or equivalently
use match(shouldCompile).with(true, () => mergeCompileTargets(...)).with(false,
() => config).run(); keep the same variables and call to mergeCompileTargets and
ensure mergedConfig is declared/initialized before the conditional.
In `@packages/config/package.json`:
- Around line 32-35: The package.json exports removed the public subpath
"./loader" which breaks consumers; restore a compatibility alias by adding an
exports entry for "./loader" that points to the compiled implementation (e.g.,
"./dist/loader.js") and its types (e.g., "./dist/loader.d.ts") so existing
imports of "@kidd-cli/config/loader" continue to work, or if you intend a
breaking change, clearly version-bump to a major release and document the
removal.
In `@packages/utils/src/index.ts`:
- Around line 2-3: Reintroduce a compatibility bridge in
packages/utils/src/index.ts by re-exporting the old identifiers clients may rely
on: add type alias export for the removed AsyncResult (e.g., export type {
ResultAsync as AsyncResult } from './fp/index.js') and re-export the filesystem
symbols under their previous root names (e.g., export { exists, list, read,
remove, write } from './node/fs.js' or provide legacy aliases if names changed),
and mark these exports with a short deprecation comment so consumers can migrate
before removal; ensure the bridge keeps the existing new exports (Result,
ResultAsync) intact and does not add new runtime dependencies.
In `@packages/utils/src/node/process.ts`:
- Around line 52-56: The spawn function currently conflates process launch
errors with a normal exit code of 1; update spawn to use the Result tuple
pattern required by the project (e.g., Promise<[number | null, Error | null]>),
wire the child process 'error' event to return [null, err] and the
'exit'/'close' handler to return [exitCode, null], and adjust the function body
around the spawn(...) call and handlers (the spawn function and its internal
'error'/'exit' handlers) so that actual launch failures propagate the Error in
the tuple instead of returning 1; also update any callers of spawn to handle the
[data, error] tuple.
- Line 26: The exported function exec currently uses two positional parameters
(cmd: string, args: readonly string[] = []) which violates the repo rule that
functions with 2+ params must accept an object; change exec to accept a single
object parameter (e.g., { cmd: string, args?: readonly string[] }) preserving
the default empty array behavior and return type ResultAsync<ExecOutput>, update
all internal callers to pass an object (e.g., exec({ cmd, args })) and adjust
any related type imports/usages to match the new signature, and run/adjust tests
or usages that referenced the old positional signature.
---
Outside diff comments:
In `@packages/bundler/src/compile/compile.ts`:
- Around line 109-124: The code calls params.lifecycle.onStepFinish only when
compileSingleTarget returns success, leaving the lifecycle open on failures; fix
by ensuring params.lifecycle.onStepFinish is always invoked for a started
target: wrap the compileSingleTarget call in a try/finally (or ensure a finally
block) and in the finally call params.lifecycle.onStepFinish({ phase: 'compile',
step: 'target', meta }) if params.lifecycle.onStepFinish is defined (keeping
existing onStepStart handling), so compileSingleTarget,
params.lifecycle.onStepStart, and params.lifecycle.onStepFinish are used as the
points to locate changes.
In `@packages/cli/src/lib/detect.ts`:
- Around line 87-99: The parsed JSON from jsonParse (variables parseError and
data) must be validated with Zod before casting to PackageJson; update the block
in detect.ts to run a Zod schema (or z.object({...}) for the PackageJson shape)
against data and if validation fails return the same read_error shape (include
validation errors in the message and filePath) instead of blindly returning
ok(data as PackageJson); ensure you reference jsonParse, parseError, data, ok(),
and PackageJson when implementing the validation and error-return path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 648f3654-2a65-425d-b13e-4907e0f697aa
⛔ Files ignored due to path filters (3)
.changeset/fix-bundler-transitive-deps.mdis excluded by!.changeset/**.changeset/upgrade-deps.mdis excluded by!.changeset/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (79)
packages/bundler/package.jsonpackages/bundler/src/autoloader/autoload-plugin.test.tspackages/bundler/src/autoloader/autoload-plugin.tspackages/bundler/src/build/build.test.tspackages/bundler/src/build/build.tspackages/bundler/src/build/bun-config.test.tspackages/bundler/src/build/bun-config.tspackages/bundler/src/build/bun-runner.tspackages/bundler/src/build/clean.test.tspackages/bundler/src/build/clean.tspackages/bundler/src/build/config.test.tspackages/bundler/src/build/config.tspackages/bundler/src/build/plugins.test.tspackages/bundler/src/build/plugins.tspackages/bundler/src/build/watch.test.tspackages/bundler/src/build/watch.tspackages/bundler/src/bundler.tspackages/bundler/src/compile/compile.test.tspackages/bundler/src/compile/compile.tspackages/bundler/src/config/detect-build-entry.test.tspackages/bundler/src/config/read-version.test.tspackages/bundler/src/config/read-version.tspackages/bundler/src/constants.tspackages/bundler/src/index.tspackages/bundler/src/types.tspackages/bundler/src/utils/resolve-build-entry.test.tspackages/bundler/src/utils/resolve-build-entry.tspackages/bundler/src/utils/resolve-config.test.tspackages/bundler/src/utils/resolve-config.tspackages/bundler/tsdown.config.tspackages/cli/src/commands/add/command.test.tspackages/cli/src/commands/add/command.tspackages/cli/src/commands/build.test.tspackages/cli/src/commands/build.tspackages/cli/src/commands/commands.test.tspackages/cli/src/commands/commands.tspackages/cli/src/commands/dev.test.tspackages/cli/src/commands/dev.tspackages/cli/src/commands/doctor.test.tspackages/cli/src/commands/doctor.tspackages/cli/src/commands/run.tspackages/cli/src/lib/checks.test.tspackages/cli/src/lib/checks.tspackages/cli/src/lib/config-helpers.tspackages/cli/src/lib/detect.tspackages/cli/src/lib/render.tspackages/cli/src/lib/write.tspackages/cli/src/manifest.tspackages/config/package.jsonpackages/config/src/index.tspackages/config/src/types.tspackages/config/src/utils/compile.tspackages/config/src/utils/index.tspackages/config/src/utils/loader.test.tspackages/config/src/utils/loader.tspackages/config/src/utils/schema.test.tspackages/config/src/utils/schema.tspackages/config/tsdown.config.tspackages/core/src/cli.test.tspackages/core/src/cli.tspackages/core/src/middleware/auth/context.tspackages/core/src/middleware/auth/types.tspackages/core/src/middleware/icons/context.tspackages/core/src/middleware/icons/install.tspackages/core/src/middleware/icons/list-system-fonts.tspackages/core/src/middleware/icons/types.tspackages/core/src/runtime/runtime.tspackages/core/src/runtime/types.tspackages/utils/package.jsonpackages/utils/src/fp/result.test.tspackages/utils/src/fp/result.tspackages/utils/src/fs.tspackages/utils/src/index.tspackages/utils/src/manifest.tspackages/utils/src/node/fs.test.tspackages/utils/src/node/fs.tspackages/utils/src/node/index.tspackages/utils/src/node/process.tspackages/utils/tsdown.config.ts
💤 Files with no reviewable changes (10)
- packages/bundler/src/config/detect-build-entry.test.ts
- packages/bundler/tsdown.config.ts
- packages/bundler/src/config/read-version.ts
- packages/utils/src/fs.ts
- packages/bundler/src/config/read-version.test.ts
- packages/bundler/src/build/bun-config.ts
- packages/bundler/src/build/bun-runner.ts
- packages/bundler/src/build/plugins.test.ts
- packages/bundler/src/build/plugins.ts
- packages/bundler/src/build/bun-config.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bundler/src/utils/clean.ts`:
- Around line 42-44: Replace the ternary that sets binaryNames with a ts-pattern
match: import match from 'ts-pattern' and use match(params.compile).with(true,
() => buildBinaryNames(params.resolved.compile.name,
params.resolved.compile.targets)).otherwise(() => new Set<string>()); do the
same refactor for the other two ternary occurrences flagged by the linter in
this file (the ones around lines 89-91 and the single-expression at line 97) —
convert each ternary to a match(...) with .with(...) and .otherwise(...) so
behavior remains identical while satisfying the no-ternary rule.
- Around line 46-55: The code currently calls fs.remove inside the Promise.all
map and unconditionally returns {type: 'removed', name} even if fs.remove
throws; wrap the removal in a try/catch inside the entries.map callback
(referencing isBuildArtifact, binaryNames, fs.remove and
params.resolved.buildOutDir) and only return {type: 'removed', name} when
fs.remove succeeds, otherwise return a non-removed result (e.g., {type: 'failed'
or 'foreign', name}) and surface/log the caught error so CleanResult.removed
only contains actually-removed items.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e900bba1-69e7-4b33-b05c-7bc87d950487
📒 Files selected for processing (5)
packages/bundler/src/build/build.tspackages/bundler/src/build/clean.test.tspackages/bundler/src/build/clean.tspackages/bundler/src/utils/clean.test.tspackages/bundler/src/utils/clean.ts
💤 Files with no reviewable changes (2)
- packages/bundler/src/build/clean.test.ts
- packages/bundler/src/build/clean.ts
- Replace 3 ternaries in clean.ts with match().exhaustive() - Replace 1 ternary in cli build.ts with match().exhaustive() - Fix array-callback-return: .map() → .reduce() in buildBinaryNames Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/commands/build.ts (1)
169-169:⚠️ Potential issue | 🟠 MajorZod schema should validate
targetsagainst actualCompileTargetvalues, not arbitrary strings.The schema at line 17 accepts any string (
z.array(z.string())), butCompileTargetis a finite literal union (e.g.,'darwin-arm64','linux-x64'). The assertion at line 169 bypasses type checking; invalid targets would pass validation at the CLI boundary and fail downstream inmapCompileTarget().Refine the schema to enumerate valid targets: use
.refine()to validate againstcompileTargets, or derive a literal union from the constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/build.ts` at line 169, The CLI Zod schema currently accepts any string for targets and then casts params.targets to CompileTarget when building (targets: params.targets as CompileTarget[]), which lets invalid values through and breaks mapCompileTarget(); update the Zod schema that validates the CLI args (the schema that currently uses z.array(z.string())) to validate against the real CompileTarget set by either: (a) deriving a Zod enum/literal union from the compileTargets constant or CompileTarget union, or (b) keeping z.array(z.string()) but adding .refine(values => values.every(v => compileTargets.includes(v)), { message: 'invalid compile target' }); ensure the validated value is typed as CompileTarget[] so you can remove the unsafe assertion in the build step where params.targets is passed to mapCompileTarget().
♻️ Duplicate comments (1)
packages/bundler/src/utils/clean.ts (1)
48-57:⚠️ Potential issue | 🟠 Major
fs.removefailure silently misreports item as removed.The
fs.removecall doesn't check its Result tuple. If removal fails (permissions, file locked, etc.), the entry is still categorized asremovedeven though it remains on disk. This breaks the contract ofCleanResult.removed.Line 38 correctly handles
fs.list's Result—apply the same pattern here.🔧 Proposed fix
const results = await Promise.all( entries.map(async (name) => { const shouldRemove = isBuildArtifact(name) || binaryNames.has(name) if (shouldRemove) { - await fs.remove(join(params.resolved.buildOutDir, name)) - return { type: 'removed' as const, name } + const [removeErr] = await fs.remove(join(params.resolved.buildOutDir, name)) + if (removeErr) { + return { type: 'foreign' as const, name } + } + return { type: 'removed' as const, name } } return { type: 'foreign' as const, name } }) )As per coding guidelines: "represent expected failures with Result tuples... Always destructure and check the error element before using the value element" (contributing/standards/typescript/errors.md).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bundler/src/utils/clean.ts` around lines 48 - 57, The code currently calls fs.remove inside entries.map and unconditionally returns {type: 'removed', name} even if fs.remove failed; change the async removal to mirror the fs.list handling by awaiting fs.remove and destructuring its Result tuple (e.g., const [rmErr] = await fs.remove(join(params.resolved.buildOutDir, name))), check rmErr before reporting removal, and only return {type: 'removed', name} when rmErr is falsy; otherwise return a non-removed result (e.g., {type: 'foreign'|'error', name, error: rmErr}) so CleanResult.removed correctly reflects actual deletions. Ensure this logic is applied inside the entries.map async callback and reference isBuildArtifact, binaryNames, and params.resolved.buildOutDir when locating the target to remove.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/build.ts`:
- Around line 54-57: The spinner handlers onStepStart and onStepFinish access
meta.label unsafely (StepEvent's meta is Readonly<Record<string, unknown>>), so
coerce or validate it before use: update the onStepStart and onStepFinish
callbacks to read meta.label with a type guard or fallback (e.g., const label =
typeof meta?.label === 'string' ? meta.label : 'step' or String(meta?.label ??
'step')) and pass that safe label into ctx.status.spinner.message to avoid
"undefined" or malformed messages; ensure both handlers use the same safeLabel
logic.
---
Outside diff comments:
In `@packages/cli/src/commands/build.ts`:
- Line 169: The CLI Zod schema currently accepts any string for targets and then
casts params.targets to CompileTarget when building (targets: params.targets as
CompileTarget[]), which lets invalid values through and breaks
mapCompileTarget(); update the Zod schema that validates the CLI args (the
schema that currently uses z.array(z.string())) to validate against the real
CompileTarget set by either: (a) deriving a Zod enum/literal union from the
compileTargets constant or CompileTarget union, or (b) keeping
z.array(z.string()) but adding .refine(values => values.every(v =>
compileTargets.includes(v)), { message: 'invalid compile target' }); ensure the
validated value is typed as CompileTarget[] so you can remove the unsafe
assertion in the build step where params.targets is passed to
mapCompileTarget().
---
Duplicate comments:
In `@packages/bundler/src/utils/clean.ts`:
- Around line 48-57: The code currently calls fs.remove inside entries.map and
unconditionally returns {type: 'removed', name} even if fs.remove failed; change
the async removal to mirror the fs.list handling by awaiting fs.remove and
destructuring its Result tuple (e.g., const [rmErr] = await
fs.remove(join(params.resolved.buildOutDir, name))), check rmErr before
reporting removal, and only return {type: 'removed', name} when rmErr is falsy;
otherwise return a non-removed result (e.g., {type: 'foreign'|'error', name,
error: rmErr}) so CleanResult.removed correctly reflects actual deletions.
Ensure this logic is applied inside the entries.map async callback and reference
isBuildArtifact, binaryNames, and params.resolved.buildOutDir when locating the
target to remove.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5fc3ad77-ee48-4c8f-a874-4c5b3e331e8b
📒 Files selected for processing (2)
packages/bundler/src/utils/clean.tspackages/cli/src/commands/build.ts
- Fix linux-x64-musl target mapping to bun-linux-x64-musl (was glibc) - Handle fs.remove failure in clean.ts (report as foreign on error) - Use ts-pattern match in resolve-build-entry.ts - Convert exec() to object params per coding standards - Add meta.label fallback for type safety in build.ts spinner - Add compile=true test coverage for config.test.ts - Collapse _require intermediate variable in config.ts Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/build.ts (1)
155-169:⚠️ Potential issue | 🟠 MajorValidate CLI compile targets against the config package's schema.
params.targetsoriginates fromz.array(z.string()).optional()and is then cast unsafely toCompileTarget[]at line 166. This lets invalid target names pass type checking and fail later in bundler logic. The config package definesCompileTargetSchema(inpackages/config/src/utils/schema.ts) but marks it private and does not export it. Either export the schema or the valid targets list from the config package and use it to validate the CLI value at the boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/build.ts` around lines 155 - 169, mergeCompileTargets is unsafely casting params.targets (from z.array(z.string()).optional()) to CompileTarget[] which allows invalid target names to slip through; update mergeCompileTargets to validate params.targets against the config package's canonical schema or exported valid-target list before assigning to compile.targets. Either export CompileTargetSchema or a validated string list from the config package (from packages/config/src/utils/schema.ts), import that into packages/cli/src/commands/build.ts, run validation on params.targets (reject or map invalid entries) and only then set compile.targets (instead of the direct cast to CompileTarget[]) so the CLI boundary enforces the config's allowed target values.packages/bundler/src/compile/compile.ts (1)
64-68:⚠️ Potential issue | 🔴 CriticalResult tuple destructuring breaks type narrowing — fix by keeping tuples intact until after checked.
After destructuring
[error, value] = resultTuple, both members remain independently nullable. Lines 155–165 usebunTargetwithout guards after themapErrorcheck; lines 266–271 useentriesafter checkinglistError; lines 64–68 pass a nullable error toerr(). Narrow the tuple itself before unpacking:if (result[0]) return err(result[0]); const [, value] = result.Also applies to: 153–165, 263–271
Lifecycle hook is asymmetric:
onStepStartalways fires, butonStepFinishonly on success (line 127). This leaves failed compilation steps unclosed, breaking step tracking in the UI. CallonStepFinishunconditionally or document why failures skip it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bundler/src/compile/compile.ts` around lines 64 - 68, The tuple destructuring is breaking type narrowing: don’t destructure result tuples until you’ve checked result[0] exists; instead test the tuple as a whole (e.g., if (result[0]) return err(result[0]); then const [, value] = result) wherever you currently do `[error, value] = result` (fix occurrences that reference bunTarget after mapError, entries after listError, and the results.find(...) usage that currently passes a nullable to err()). Also make the lifecycle symmetric: ensure onStepFinish is invoked unconditionally (e.g., in a finally block) for each onStepStart call so failed compilation steps still close (adjust the code paths that call onStepStart/onStepFinish accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bundler/src/build/config.test.ts`:
- Line 1: The test is calling .map() directly on output.plugins (an
InlineConfig.plugins / RolldownPluginOption<any>) which may include falsy,
nested, or async entries; before mapping plugin names, normalize and narrow
output.plugins to a plain Plugin[]: convert to an array if undefined, flatten
nested arrays, filter out falsy values, and cast to the Rollup Plugin type (or
resolve async entries if your test expects promises) — then call .map(...) on
that narrowed plugins array instead of directly on output.plugins.
In `@packages/bundler/src/build/config.ts`:
- Around line 98-100: The returned ALWAYS_BUNDLE array is being returned by
reference from the match in alwaysBundle, causing shared-state leakage; change
the branch that currently returns ALWAYS_BUNDLE to return a shallow clone (e.g.,
spread into a new array) so each config gets its own instance — update the match
for compile (symbol: alwaysBundle, compile, match) to clone ALWAYS_BUNDLE before
returning instead of returning the module-level array directly.
In `@packages/bundler/src/compile/compile.ts`:
- Around line 127-129: The code only calls params.lifecycle.onStepFinish when
result[0] === null, leaving started targets without a matching finish; ensure
onStepFinish is always invoked for targets that had onStepStart called. Update
the logic around the target lifecycle so that when you call
params.lifecycle.onStepStart(...) you set a local flag (e.g., targetStarted) and
then, instead of checking result[0] === null, always await
params.lifecycle.onStepFinish({ phase: 'compile', step: 'target', meta }) when
targetStarted is true and params.lifecycle.onStepFinish exists; keep using the
same meta object so consumers can correlate start/finish.
In `@packages/utils/src/node/process.ts`:
- Around line 25-31: The exec function currently calls execFile without a cwd,
causing child processes to inherit the parent working directory; update exec to
accept an optional cwd (e.g., add cwd?: string to the params object for exec)
and pass it into the execFile options when invoking execFile, so execFile(cmd, {
cwd, args: [...] }, …) is used; then ensure callers (such as compileSingleTarget
in packages/bundler/src/compile/compile.ts) pass params.resolved.cwd into exec
so cleanBunBuildArtifacts can reliably clean the same directory.
---
Outside diff comments:
In `@packages/bundler/src/compile/compile.ts`:
- Around line 64-68: The tuple destructuring is breaking type narrowing: don’t
destructure result tuples until you’ve checked result[0] exists; instead test
the tuple as a whole (e.g., if (result[0]) return err(result[0]); then const [,
value] = result) wherever you currently do `[error, value] = result` (fix
occurrences that reference bunTarget after mapError, entries after listError,
and the results.find(...) usage that currently passes a nullable to err()). Also
make the lifecycle symmetric: ensure onStepFinish is invoked unconditionally
(e.g., in a finally block) for each onStepStart call so failed compilation steps
still close (adjust the code paths that call onStepStart/onStepFinish
accordingly).
In `@packages/cli/src/commands/build.ts`:
- Around line 155-169: mergeCompileTargets is unsafely casting params.targets
(from z.array(z.string()).optional()) to CompileTarget[] which allows invalid
target names to slip through; update mergeCompileTargets to validate
params.targets against the config package's canonical schema or exported
valid-target list before assigning to compile.targets. Either export
CompileTargetSchema or a validated string list from the config package (from
packages/config/src/utils/schema.ts), import that into
packages/cli/src/commands/build.ts, run validation on params.targets (reject or
map invalid entries) and only then set compile.targets (instead of the direct
cast to CompileTarget[]) so the CLI boundary enforces the config's allowed
target values.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fafef126-e6e8-4caf-9ea7-94f2364eebf8
📒 Files selected for processing (8)
packages/bundler/src/build/config.test.tspackages/bundler/src/build/config.tspackages/bundler/src/compile/compile.test.tspackages/bundler/src/compile/compile.tspackages/bundler/src/utils/clean.tspackages/bundler/src/utils/resolve-build-entry.tspackages/cli/src/commands/build.tspackages/utils/src/node/process.ts
- Fix typecheck: cast plugins to typed array in config.test.ts - Clone ALWAYS_BUNDLE to prevent shared state mutation - Always fire onStepFinish for symmetry with onStepStart - Add optional cwd to exec() and thread through compile Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bundler/src/build/config.ts`:
- Around line 138-144: Replace the two-branch imperative if/return in
buildPlugins (and similarly in resolveId and load) with ts-pattern's match()
expression: use match(compile).with(true, ...) and .otherwise(...) to return
[createStubPlugin(STUB_PACKAGES)] when compile is true and [] otherwise; follow
the same pattern used by buildDeps and buildDefine to keep conditional logic
functional and consistent with the repo style.
In `@packages/bundler/src/compile/compile.test.ts`:
- Around line 36-38: The test narrows compile.targets using an overly broad "as
never"; replace that with an explicit type cast to the expected CompileTarget
array type and import the type: add "import type { CompileTarget } from
'@kidd-cli/config'" at the top and change the cast on compile.targets to use "as
CompileTarget[]" (or the precise expected CompileTarget form) so the targets
field has the correct typed value in the compile object.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cc3c3a59-f7ce-42ed-8f14-63c8d2180d49
📒 Files selected for processing (5)
packages/bundler/src/build/config.test.tspackages/bundler/src/build/config.tspackages/bundler/src/compile/compile.test.tspackages/bundler/src/compile/compile.tspackages/utils/src/node/process.ts
- Replace if/return with match() in buildPlugins, resolveId, load - Tighten type cast from `as never` to `as readonly CompileTarget[]` Co-Authored-By: Claude <noreply@anthropic.com>
Add build.define to kidd.config.ts for explicit compile-time constants. Auto-resolve KIDD_PUBLIC_* env vars from process.env at build time (same convention as Next.js NEXT_PUBLIC_*). Show injected vars in build output log. Closes #145 Co-Authored-By: Claude <noreply@anthropic.com>
… migration - Binary runner: use example name instead of hardcoded 'cli' - Node runner: default to dist/index.mjs (tsdown ESM output) - authenticated-service: fix custom binary name and entry path - CI: update chmod to match new binary naming convention - Benchmarks: update codspeed entry to index.mjs Co-Authored-By: Claude <noreply@anthropic.com>
Merging this PR will improve performance by 16.66%
Performance Changes
Comparing 
Footnotes
|
Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tsdown → index.mjs → bun build --compile)createBundler({ config, cwd, ...lifecycle })replaces standalonebuild/watch/compileexportsonStart,onFinish,onStepStart,onStepFinishwith phase/step/meta eventscreateBundler) + 4 types@kidd-cli/utils/nodewithfs.*andprocess.*async wrappers@kidd-cli/config/utilssubpath,compileTargetssingle source of truth with{ target, label, default }ResultAsyncreplacesAsyncResultacross all packagesFiles changed
82 files, +1647 / -2190 (net -543 lines)
Deleted
bun-runner.ts,bun-config.ts,plugins.ts— Bun.build architectureread-version.ts— manifest read once in factory insteaddetect-build-entry.test.ts— merged intoresolve-build-entry.test.tsCreated
bundler.ts—createBundlerfactory with lifecycle hooksutils/resolve-build-entry.ts— async entry detectionutils/node/fs.ts—exists,read,write,list,mkdir,removeutils/node/process.ts—exec,spawn,existsconfig/utils/compile.ts—compileTargetsarray,CompileTargettypeconfig/utils/index.ts— barrel for internal config exportsTest plan
pnpm checkpasses (typecheck + lint + format)pnpm test --filter='./packages/*'— all package tests passkidd buildproduces correct output in exampleskidd build --compileproduces standalone binarieskidd devwatch mode works