Add Ultracite and intent to init flow#264
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 03a7538 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 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 |
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThis PR restricts Node.js engine support to versions 22, 24, and 26 across the monorepo; introduces a new parseCommandString utility for shell-style command tokenization with quote and escape support; centralizes NODE_RUNTIME_VERSION; refactors CLI init and execute paths to use the parser for package-manager commands; and updates tests and CI workflows. ChangesNode Engine Support and CLI Command Parsing
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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/cli/init.ts (1)
448-455:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLint command error handling differs from executeInitPlan.
In
executeInitPlan.ts(lines 453-464), the lint command is wrapped withEffect.eitherand failures log a warning but continue setup. Here ininit.ts, the lint command has no error handling and will throw on failure, creating inconsistent behavior between the two execution paths.🔧 Align error handling with executeInitPlan
- const [lintCommand, ...lintArgs] = parseCommandString(formatPackageManagerCommand(pkgManager, "lint")); - if (!lintCommand) { - throw new Error(`Unable to resolve lint command for ${pkgManager}.`); - } - await execa(lintCommand, lintArgs, { - cwd: projectDir, - stdio: "pipe", - }); + const [lintCommand, ...lintArgs] = parseCommandString(formatPackageManagerCommand(pkgManager, "lint")); + if (!lintCommand) { + throw new Error(`Unable to resolve lint command for ${pkgManager}.`); + } + await execa(lintCommand, lintArgs, { + cwd: projectDir, + stdio: "pipe", + }).catch((error: unknown) => { + if (state.debug) { + console.log("Lint did not succeed; continuing setup."); + console.error(error); + } + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/init.ts` around lines 448 - 455, The lint execution in init.ts uses execa directly (via parseCommandString/formatPackageManagerCommand) and will throw on failure, unlike executeInitPlan.ts which wraps the lint step with Effect.either and only logs a warning; change init.ts to match that behavior by catching errors from the execa call (or using the same Effect.either pattern) and logging a non-fatal warning instead of letting the error propagate, keeping the rest of the initialization flow continuing; keep references to parseCommandString, formatPackageManagerCommand and the execa invocation so the replacement targets the same lintCommand/lintArgs execution site.
🧹 Nitpick comments (1)
packages/cli/src/cli/init.ts (1)
443-444: 💤 Low valueReplace console.log with proper logging service.
The coding guidelines state to remove
console.logfrom production code. Even though these are gated behindstate.debug, prefer using a proper logging abstraction for consistency.📝 Suggested approach
If a debug logging method exists elsewhere in the codebase:
- if (state.debug) { - console.log(`Fix command failed; continuing. packageManager=${pkgManager} command=${fixCommandString}`); - console.error(error); - } + if (state.debug) { + logger.debug(`Fix command failed; continuing. packageManager=${pkgManager} command=${fixCommandString}`, error); + }Otherwise, document why direct console usage is acceptable in debug mode.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/cli/init.ts` around lines 443 - 444, Replace the direct console calls in the failure path with the project's logging abstraction: instead of console.log and console.error in the block that prints "Fix command failed; continuing. packageManager=${pkgManager} command=${fixCommandString}", call the appropriate logger (e.g., logger.debug or logger.error) so the message respects state.debug and centralized logging; pass contextual fields pkgManager and fixCommandString and include the error object when logging the failure. If no logger exists in this module, import or create the shared CLI logger used elsewhere, and ensure the debug-level message is gated by state.debug via the logger rather than console.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/consts.ts`:
- Line 10: NODE_RUNTIME_VERSION is currently set to "^24.11.0", which restricts
generated projects to Node 24.x; change the export const NODE_RUNTIME_VERSION to
the multi-version range "^22.0.0 || ^24.0.0 || ^26.0.0" so that engines.node and
devEngines.runtime.version in scaffolded projects support Node 22, 24, and 26
(update the NODE_RUNTIME_VERSION constant in consts.ts accordingly).
In `@packages/cli/src/core/executeInitPlan.ts`:
- Around line 440-441: The comment above the lint/runFix block is inaccurate: it
says "lint errors still propagate below" but the code uses Effect.either around
the lint command and logs a warning on failure (so lint errors are tolerated);
update the comment near plan.tasks.runFix / getPackageScriptCommand /
processService.run / Effect.either to state that lint failures are caught and
only logged as warnings (non-blocking), while other errors still propagate as
intended.
In `@packages/cli/src/utils/projectFiles.ts`:
- Line 58: Add an explicit return type annotation to the exported function
parseCommandString so the signature clearly declares the function's return type;
update the function declaration in packages/cli/src/utils/projectFiles.ts to
include the precise return type that matches its implementation (for example
string, string[], or a specific ParsedCommand type/interface) and adjust or
export any related types if needed to keep type compatibility.
In `@packages/cli/template/vite-wv/package.json`:
- Around line 6-8: The engines field in the package.json template currently pins
"node": ">=20.11.0", which is inconsistent with the monorepo; update the
"engines" -> "node" value in packages/cli/template/vite-wv/package.json to match
the repo constraint by setting it to "^22.0.0 || ^24.0.0 || ^26.0.0" so
scaffolded projects require the same Node ranges as the CLI and other packages.
---
Outside diff comments:
In `@packages/cli/src/cli/init.ts`:
- Around line 448-455: The lint execution in init.ts uses execa directly (via
parseCommandString/formatPackageManagerCommand) and will throw on failure,
unlike executeInitPlan.ts which wraps the lint step with Effect.either and only
logs a warning; change init.ts to match that behavior by catching errors from
the execa call (or using the same Effect.either pattern) and logging a non-fatal
warning instead of letting the error propagate, keeping the rest of the
initialization flow continuing; keep references to parseCommandString,
formatPackageManagerCommand and the execa invocation so the replacement targets
the same lintCommand/lintArgs execution site.
---
Nitpick comments:
In `@packages/cli/src/cli/init.ts`:
- Around line 443-444: Replace the direct console calls in the failure path with
the project's logging abstraction: instead of console.log and console.error in
the block that prints "Fix command failed; continuing.
packageManager=${pkgManager} command=${fixCommandString}", call the appropriate
logger (e.g., logger.debug or logger.error) so the message respects state.debug
and centralized logging; pass contextual fields pkgManager and fixCommandString
and include the error object when logging the failure. If no logger exists in
this module, import or create the shared CLI logger used elsewhere, and ensure
the debug-level message is gated by state.debug via the logger rather than
console.
🪄 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: CHILL
Plan: Pro
Run ID: 173d35b6-4425-419a-9d33-8733b74051d1
📒 Files selected for processing (24)
.changeset/node-engine-support.md.changeset/quiet-command-parsing.md.github/workflows/check-skills.yml.github/workflows/validate-skills.ymlpackage.jsonpackages/cli-old/package.jsonpackages/cli/src/cli/init.tspackages/cli/src/consts.tspackages/cli/src/core/executeInitPlan.tspackages/cli/src/core/planInit.tspackages/cli/src/helpers/intent.tspackages/cli/src/helpers/ultracite.tspackages/cli/src/utils/projectFiles.tspackages/cli/template/vite-wv/package.jsonpackages/cli/template/vite-wv/src/routes/query-demo.tsxpackages/cli/tests/executor.test.tspackages/cli/tests/init-run-init-regression.test.tspackages/cli/tests/init-scaffold-contract.test.tspackages/cli/tests/integration.test.tspackages/cli/tests/planner.test.tspackages/cli/tests/test-layer.tspackages/create-proofkit/package.jsonpackages/fmdapi/package.jsonpackages/fmodata/package.json
- parse package-manager commands safely - fix lint step handling in init flow - raise Node engine ranges to 22/24/26
- warn on lint failure during init - accept Node 22/24/26 in CLI and template - add parseCommandString return type
eluce2
force-pushed
the
feature/init-ultracite-intent
branch
from
7559304 to
03a7538
Compare
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
fixthenlint, with safer command parsing.Testing
pnpm run cifix/lintexecution checkSummary by CodeRabbit
Breaking Changes
New Features
Improvements
Chores