Add Ultracite and TanStack Intent to init flow#263
Merged
Conversation
- scaffold Ultracite, TanStack Intent, and npm/pnpm policy files - run fix/lint post-install and add Node engine pins - refresh template deps and cleanup init scaffolds
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: 171bfec The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds Node runtime metadata and package-manager config emission to init plans/package.json, runs Ultracite and TanStack Intent during scaffolding, resolves and runs package-manager-specific fix/lint steps, modernizes templates/scripts, and updates tests to assert new behavior. ChangesCLI init / planning / execution
Package-manager config, dependency updates, and planner outputs
Template modernization, small API shape moves, and utilities
Tests & test helpers
Sequence DiagramsequenceDiagram
actor User
participant CLI
participant Planner
participant Executor
participant PM as PackageManager
participant Ultracite
participant Intent
participant Linter
User->>CLI: run init
CLI->>Planner: planInit(appType, pkgManager)
Planner->>Planner: include runtime ^24.11.0, decide writes (.npmrc / pnpm-workspace)
Planner-->>CLI: InitPlan (commands/tasks/writes)
CLI->>Executor: executeInitPlan(plan)
Executor->>Executor: scaffold files & replace placeholders
Executor->>PM: install deps (if !noInstall)
Executor->>Ultracite: run getUltraciteInitCommand(appType, pkgManager)
Ultracite-->>Executor: {command,args}
Executor->>PM: run ultracite init (may skip install)
PM->>Ultracite: initialize (oxlint presets)
Ultracite->>Executor: done
Executor->>Executor: write oxlint.config.ts (browser)
Executor->>Intent: getIntentInstallCommand(pkgManager)
Intent-->>Executor: {command,args}
Executor->>PM: run intent install
PM->>Intent: install configured agents
Intent->>Executor: done
Executor->>Executor: resolve fix via getPackageScriptCommand
Executor->>PM: run fix (errors swallowed)
Executor->>Executor: resolve lint via getPackageScriptCommand
Executor->>PM: run lint (errors warned/propagated)
PM->>Linter: lint results
Linter->>Executor: done / error
Executor-->>User: project ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 5
🧹 Nitpick comments (4)
packages/cli/template/vite-wv/src/routes/query-demo.tsx (1)
10-10: ⚡ Quick winUse a const-asserted query key for stronger type safety.
Line 10 currently widens to
string[]; usingas constpreserves literal tuple typing and improves query-key consistency.♻️ Proposed refactor
+const STARTER_CONNECTION_HINT_QUERY_KEY = ["starter-connection-hint"] as const; + export const QueryDemoPage = () => { const hintQuery = useQuery({ queryFn: getConnectionHint, - queryKey: ["starter-connection-hint"], + queryKey: STARTER_CONNECTION_HINT_QUERY_KEY, });As per coding guidelines, "Use const assertions (
as const) for immutable values and literal types".🤖 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/template/vite-wv/src/routes/query-demo.tsx` at line 10, The query key is currently declared as a plain array which widens to string[]; update the declaration that sets queryKey (in query-demo.tsx) to use a const assertion so the key remains a literal tuple — change the queryKey assignment to use "as const" (i.e., make queryKey: ["starter-connection-hint"] as const) to preserve strong typing and consistent query-key behavior.packages/cli/src/cli/init.ts (1)
434-442: ⚡ Quick winFix command errors are swallowed silently without logging.
Line 442 uses
.catch(() => undefined)to suppress all errors from the fix command. While this may be intentional (fix is non-critical), completely silent failure makes debugging difficult. Consider logging the error at debug level.🔍 Suggested improvement
await execa(fixCommand, fixArgs, { cwd: projectDir, stdio: "pipe", - }).catch(() => undefined); + }).catch((error) => { + // Fix command is non-critical; log but continue + if (state.debug) { + console.debug("Fix command failed (non-critical):", error.message); + } + });🤖 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 434 - 442, The catch on the execa call in the init flow swallows all errors (when running the fix command) making failures invisible; update the promise handling around execa(fixCommand, fixArgs, { cwd: projectDir, stdio: "pipe" }) so that failures are caught but logged at debug (or trace) level instead of ignored—use the existing logger or processLogger (or a debug wrapper) to log the caught error and context including pkgManager and command generated by formatPackageManagerCommand, while preserving non-fatal behavior (do not rethrow).packages/cli/src/core/planInit.ts (1)
28-28: ⚡ Quick winCentralize NODE_RUNTIME_VERSION constant.
NODE_RUNTIME_VERSIONis defined here and also inpackages/cli/src/cli/init.ts(line 150). This duplication increases maintenance risk if the version needs to be updated.♻️ Suggested refactor
Extract to a shared constants file:
// In ~/consts.ts or similar export const NODE_RUNTIME_VERSION = "^24.11.0";Then import in both files:
+ import { NODE_RUNTIME_VERSION } from "~/consts.js"; - const NODE_RUNTIME_VERSION = "^24.11.0";🤖 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/core/planInit.ts` at line 28, The NODE_RUNTIME_VERSION constant is duplicated; extract NODE_RUNTIME_VERSION into a single shared constants module (e.g., export const NODE_RUNTIME_VERSION = "…") and replace the local definitions by importing that constant wherever it's used (references: NODE_RUNTIME_VERSION in planInit.ts and the other init.ts occurrence) so both files consume the centralized value and you only maintain it in one place.packages/cli/src/core/executeInitPlan.ts (1)
447-456: 💤 Low valueDocument why fix errors are silently swallowed.
Lines 449-455 wrap the
fixcommand execution inEffect.either, which discards the error result. This means fix failures are silently ignored, while lint failures (lines 458-465) propagate and can fail the init. Please add a comment explaining why fix errors are acceptable (e.g., "Fix may fail on newly scaffolded code; non-blocking").📝 Suggested comment
if (plan.tasks.runFix) { const fixCommand = getPackageScriptCommand(plan, "fix"); + // Fix command may fail on newly scaffolded projects; treat as non-blocking yield* Effect.either( processService.run(fixCommand.command, fixCommand.args, {🤖 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/core/executeInitPlan.ts` around lines 447 - 456, Add a short explanatory comment above the block that wraps the fix command in Effect.either (the code that checks plan.tasks.runFix, calls getPackageScriptCommand and invokes processService.run) stating that fix failures are intentionally non-blocking and may be silently ignored because automated fixes can fail on freshly scaffolded code or diverse dev environments; mention that lint errors still propagate and can fail the init. Keep the comment concise and refer to the symbols plan.tasks.runFix, getPackageScriptCommand, Effect.either, and processService.run so reviewers understand the exact rationale and scope.
🤖 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/cli/init.ts`:
- Around line 435-437: The current split on space of the string returned by
formatPackageManagerCommand (used to derive fixCommand and fixArgs) will break
when the command includes quoted or complex arguments; update the implementation
so you either (A) change formatPackageManagerCommand to return an array of argv
(e.g., ['cmd','arg1',...]) and use that directly where fixCommand/fixArgs are
assigned, or (B) add a shared helper (e.g., parseCommandString) that tokenizes a
shell-style command string honoring quotes/escaping and use it in init.ts (the
fixCommand/fixArgs assignment), executeInitPlan.ts and ultracite.ts to
consistently produce a command and args array; ensure callers use the parsed
array rather than String.prototype.split(" ").
In `@packages/cli/src/core/executeInitPlan.ts`:
- Around line 88-94: The code in getPackageScriptCommand incorrectly splits the
output of formatPackageManagerCommand(plan.request.packageManager, scriptName)
by a single space which breaks when arguments contain spaces or quotes; update
this by either (a) changing formatPackageManagerCommand to return a structured
value (e.g., {command, args: string[]}) and use that directly in
getPackageScriptCommand, or (b) parse the returned string with a robust
shell-aware tokenizer (e.g., a shell-words/shell-quote style parser) to properly
handle quoted and spaced arguments; ensure getPackageScriptCommand returns the
original command token as command and the remaining tokens as args and preserve
existing error behavior when no command is found.
In `@packages/cli/src/helpers/ultracite.ts`:
- Around line 10-16: splitExecuteCommand currently uses .split(" ") on
getTemplatePackageExecuteCommand(packageManager), which breaks when arguments
contain spaces or quotes; replace the naive split with a robust shell-style
tokenizer (or a small parseQuotedArgs utility) that splits into command and args
while respecting single/double quotes and escapes, then strip surrounding quotes
from arguments and return { command, args } (keep the existing error check for
missing command). Use the function name splitExecuteCommand and the source
string from getTemplatePackageExecuteCommand(packageManager) as the targets to
change.
In `@packages/cli/template/vite-wv/scripts/upload.js`:
- Line 8: The template uses import.meta.dirname which requires Node.js 20.11.0+,
so add an engines.node field to the template's package.json declaring
">=20.11.0" to prevent users on older Node versions from scaffolding; open the
package.json for the vite-wv template and add an "engines": { "node":
">=20.11.0" } entry (or update the existing engines object) so the requirement
is explicit and aligns with the use of import.meta.dirname.
In `@packages/cli/tests/init-run-init-regression.test.ts`:
- Around line 201-225: The test "writes pnpm build policy before install for
pnpm 10" currently relies on the shared execaMock version and only asserts calls
exist; update the test to explicitly set the mocked pnpm version to "10.0.0"
(override whatever shared execaMock/version setup is used for other tests) and
add an ordering assertion that the writeFileSyncMock call that writes
"pnpm-workspace.yaml" occurred before the first pnpm execaMock invocation—e.g.
set the mock pnpm version for this test via the existing mock/version helper or
by configuring execaMock to return version "10.0.0", then assert
writeFileSyncMock.mock.invocationCallOrder[0] <
execaMock.mock.invocationCallOrder[0] (and keep the existing content/assertions
for the file contents and pnpm fix/lint calls).
---
Nitpick comments:
In `@packages/cli/src/cli/init.ts`:
- Around line 434-442: The catch on the execa call in the init flow swallows all
errors (when running the fix command) making failures invisible; update the
promise handling around execa(fixCommand, fixArgs, { cwd: projectDir, stdio:
"pipe" }) so that failures are caught but logged at debug (or trace) level
instead of ignored—use the existing logger or processLogger (or a debug wrapper)
to log the caught error and context including pkgManager and command generated
by formatPackageManagerCommand, while preserving non-fatal behavior (do not
rethrow).
In `@packages/cli/src/core/executeInitPlan.ts`:
- Around line 447-456: Add a short explanatory comment above the block that
wraps the fix command in Effect.either (the code that checks plan.tasks.runFix,
calls getPackageScriptCommand and invokes processService.run) stating that fix
failures are intentionally non-blocking and may be silently ignored because
automated fixes can fail on freshly scaffolded code or diverse dev environments;
mention that lint errors still propagate and can fail the init. Keep the comment
concise and refer to the symbols plan.tasks.runFix, getPackageScriptCommand,
Effect.either, and processService.run so reviewers understand the exact
rationale and scope.
In `@packages/cli/src/core/planInit.ts`:
- Line 28: The NODE_RUNTIME_VERSION constant is duplicated; extract
NODE_RUNTIME_VERSION into a single shared constants module (e.g., export const
NODE_RUNTIME_VERSION = "…") and replace the local definitions by importing that
constant wherever it's used (references: NODE_RUNTIME_VERSION in planInit.ts and
the other init.ts occurrence) so both files consume the centralized value and
you only maintain it in one place.
In `@packages/cli/template/vite-wv/src/routes/query-demo.tsx`:
- Line 10: The query key is currently declared as a plain array which widens to
string[]; update the declaration that sets queryKey (in query-demo.tsx) to use a
const assertion so the key remains a literal tuple — change the queryKey
assignment to use "as const" (i.e., make queryKey: ["starter-connection-hint"]
as const) to preserve strong typing and consistent query-key behavior.
🪄 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: 73fce82a-6c29-424c-8ab0-34d03927777b
📒 Files selected for processing (35)
.changeset/npm-min-release-age.md.changeset/ultracite-init.mdpackages/cli/src/cli/init.tspackages/cli/src/consts.tspackages/cli/src/core/executeInitPlan.tspackages/cli/src/core/planInit.tspackages/cli/src/core/types.tspackages/cli/src/helpers/createProject.tspackages/cli/src/helpers/intent.tspackages/cli/src/helpers/logNextSteps.tspackages/cli/src/helpers/ultracite.tspackages/cli/src/installers/dependencyVersionMap.tspackages/cli/template/nextjs-shadcn/biome.jsonpackages/cli/template/nextjs-shadcn/src/app/(main)/page.tsxpackages/cli/template/nextjs-shadcn/src/components/AppLogo.tsxpackages/cli/template/nextjs-shadcn/src/components/AppShell/internal/Header.tsxpackages/cli/template/nextjs-shadcn/src/components/AppShell/internal/HeaderMobileMenu.tsxpackages/cli/template/nextjs-shadcn/src/components/AppShell/internal/HeaderNavLink.tsxpackages/cli/template/nextjs-shadcn/src/components/AppShell/slot-header-mobile-content.tsxpackages/cli/template/nextjs-shadcn/src/components/ui/button.tsxpackages/cli/template/nextjs-shadcn/src/lib/utils.tspackages/cli/template/vite-wv/package.jsonpackages/cli/template/vite-wv/scripts/filemaker.jspackages/cli/template/vite-wv/scripts/upload.jspackages/cli/template/vite-wv/src/lib/utils.tspackages/cli/template/vite-wv/src/main.tsxpackages/cli/template/vite-wv/src/router.tsxpackages/cli/template/vite-wv/src/routes/query-demo.tsxpackages/cli/template/vite-wv/vite.config.tspackages/cli/tests/executor.test.tspackages/cli/tests/init-fixtures.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.ts
💤 Files with no reviewable changes (3)
- packages/cli/template/nextjs-shadcn/biome.json
- packages/cli/template/nextjs-shadcn/src/components/AppShell/internal/Header.tsx
- packages/cli/template/nextjs-shadcn/src/app/(main)/page.tsx
Comment on lines
+435
to
+437
| const [fixCommand, ...fixArgs] = formatPackageManagerCommand(pkgManager, "fix").split(" "); | ||
| if (!fixCommand) { | ||
| throw new Error(`Unable to resolve fix command for ${pkgManager}.`); |
There was a problem hiding this comment.
Command string splitting may break with complex arguments.
Lines 435 and 444 split formatPackageManagerCommand() result on space. If the formatted command contains quoted arguments with spaces, this will incorrectly parse the command and args.
This is the same issue as in executeInitPlan.ts line 89 and ultracite.ts line 11. Consider extracting a shared helper that handles this splitting consistently, or document the assumption that formatted commands never contain complex arguments.
Also applies to: 444-446
🤖 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 435 - 437, The current split on
space of the string returned by formatPackageManagerCommand (used to derive
fixCommand and fixArgs) will break when the command includes quoted or complex
arguments; update the implementation so you either (A) change
formatPackageManagerCommand to return an array of argv (e.g.,
['cmd','arg1',...]) and use that directly where fixCommand/fixArgs are assigned,
or (B) add a shared helper (e.g., parseCommandString) that tokenizes a
shell-style command string honoring quotes/escaping and use it in init.ts (the
fixCommand/fixArgs assignment), executeInitPlan.ts and ultracite.ts to
consistently produce a command and args array; ensure callers use the parsed
array rather than String.prototype.split(" ").
Comment on lines
+88
to
+94
| function getPackageScriptCommand(plan: InitPlan, scriptName: string) { | ||
| const [command, ...args] = formatPackageManagerCommand(plan.request.packageManager, scriptName).split(" "); | ||
| if (!command) { | ||
| throw new Error(`Unable to resolve ${scriptName} command for ${plan.request.packageManager}.`); | ||
| } | ||
| return { command, args }; | ||
| } |
There was a problem hiding this comment.
Splitting formatted command on space may fail with complex arguments.
Line 89 splits the result of formatPackageManagerCommand() on a single space. If the formatted command includes arguments with embedded spaces (even quoted), this will incorrectly split the command and args array.
🛡️ Suggested defensive approach
Consider documenting the assumption or using a more robust parser:
function getPackageScriptCommand(plan: InitPlan, scriptName: string) {
+ // Note: assumes formatPackageManagerCommand returns space-delimited tokens without quoted args
const [command, ...args] = formatPackageManagerCommand(plan.request.packageManager, scriptName).split(" ");
if (!command) {
throw new Error(`Unable to resolve ${scriptName} command for ${plan.request.packageManager}.`);
}
return { command, args };
}📝 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
| function getPackageScriptCommand(plan: InitPlan, scriptName: string) { | |
| const [command, ...args] = formatPackageManagerCommand(plan.request.packageManager, scriptName).split(" "); | |
| if (!command) { | |
| throw new Error(`Unable to resolve ${scriptName} command for ${plan.request.packageManager}.`); | |
| } | |
| return { command, args }; | |
| } | |
| function getPackageScriptCommand(plan: InitPlan, scriptName: string) { | |
| // Note: assumes formatPackageManagerCommand returns space-delimited tokens without quoted args | |
| const [command, ...args] = formatPackageManagerCommand(plan.request.packageManager, scriptName).split(" "); | |
| if (!command) { | |
| throw new Error(`Unable to resolve ${scriptName} command for ${plan.request.packageManager}.`); | |
| } | |
| return { command, args }; | |
| } |
🤖 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/core/executeInitPlan.ts` around lines 88 - 94, The code in
getPackageScriptCommand incorrectly splits the output of
formatPackageManagerCommand(plan.request.packageManager, scriptName) by a single
space which breaks when arguments contain spaces or quotes; update this by
either (a) changing formatPackageManagerCommand to return a structured value
(e.g., {command, args: string[]}) and use that directly in
getPackageScriptCommand, or (b) parse the returned string with a robust
shell-aware tokenizer (e.g., a shell-words/shell-quote style parser) to properly
handle quoted and spaced arguments; ensure getPackageScriptCommand returns the
original command token as command and the remaining tokens as args and preserve
existing error behavior when no command is found.
Comment on lines
+10
to
+16
| function splitExecuteCommand(packageManager: PackageManager) { | ||
| const [command, ...args] = getTemplatePackageExecuteCommand(packageManager).split(" "); | ||
| if (!command) { | ||
| throw new Error(`Unable to resolve package execute command for ${packageManager}.`); | ||
| } | ||
| return { command, args }; | ||
| } |
There was a problem hiding this comment.
Splitting command string on space may fail with quoted arguments.
Line 11 splits getTemplatePackageExecuteCommand(packageManager) on a single space. If the execute command contains arguments with embedded spaces (even if quoted), this will incorrectly parse the command and args.
🛡️ Suggested defensive fix
Consider using a more robust parsing approach, or document that the execute command must never contain spaces in arguments:
function splitExecuteCommand(packageManager: PackageManager) {
const [command, ...args] = getTemplatePackageExecuteCommand(packageManager).split(" ");
if (!command) {
throw new Error(`Unable to resolve package execute command for ${packageManager}.`);
}
+ // Note: assumes execute command contains no quoted arguments with spaces
return { command, args };
}📝 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
| function splitExecuteCommand(packageManager: PackageManager) { | |
| const [command, ...args] = getTemplatePackageExecuteCommand(packageManager).split(" "); | |
| if (!command) { | |
| throw new Error(`Unable to resolve package execute command for ${packageManager}.`); | |
| } | |
| return { command, args }; | |
| } | |
| function splitExecuteCommand(packageManager: PackageManager) { | |
| const [command, ...args] = getTemplatePackageExecuteCommand(packageManager).split(" "); | |
| if (!command) { | |
| throw new Error(`Unable to resolve package execute command for ${packageManager}.`); | |
| } | |
| // Note: assumes execute command contains no quoted arguments with spaces | |
| return { command, args }; | |
| } |
🤖 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/helpers/ultracite.ts` around lines 10 - 16,
splitExecuteCommand currently uses .split(" ") on
getTemplatePackageExecuteCommand(packageManager), which breaks when arguments
contain spaces or quotes; replace the naive split with a robust shell-style
tokenizer (or a small parseQuotedArgs utility) that splits into command and args
while respecting single/double quotes and escapes, then strip surrounding quotes
from arguments and return { command, args } (keep the existing error check for
missing command). Use the function name splitExecuteCommand and the source
string from getTemplatePackageExecuteCommand(packageManager) as the targets to
change.
| import { deployHtml } from "./filemaker.js"; | ||
|
|
||
| const currentDirectory = fileURLToPath(new URL(".", import.meta.url)); | ||
| const currentDirectory = import.meta.dirname; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if the template package.json specifies minimum Node version
# Check for engines field in template package.json
cat packages/cli/template/vite-wv/package.json | jq -r '.engines.node // "NOT SET"'
# Also check if there's a .nvmrc or .node-version file
fd -t f '^(\.nvmrc|\.node-version)$' packages/cli/template/vite-wv/Repository: proofsh/proofkit
Length of output: 68
Add engines.node field to specify Node.js 20.11.0+ requirement.
The template uses import.meta.dirname which requires Node.js 20.11.0 or later, but the template's package.json does not specify this requirement. Add an engines field to packages/cli/template/vite-wv/package.json to prevent runtime errors when users scaffold this template on older Node versions.
🤖 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/template/vite-wv/scripts/upload.js` at line 8, The template uses
import.meta.dirname which requires Node.js 20.11.0+, so add an engines.node
field to the template's package.json declaring ">=20.11.0" to prevent users on
older Node versions from scaffolding; open the package.json for the vite-wv
template and add an "engines": { "node": ">=20.11.0" } entry (or update the
existing engines object) so the requirement is explicit and aligns with the use
of import.meta.dirname.
Comment on lines
+201
to
+225
| it("writes pnpm build policy before install for pnpm 10", async () => { | ||
| mockState.appType = "webviewer"; | ||
|
|
||
| await expect( | ||
| runInit("demo-webviewer", { | ||
| ...browserFilemakerFlags, | ||
| noInstall: false, | ||
| dataSource: "none", | ||
| }), | ||
| ).resolves.toBeUndefined(); | ||
|
|
||
| expect(writeFileSyncMock).toHaveBeenCalledWith( | ||
| "/tmp/proofkit-regression/demo-browser/pnpm-workspace.yaml", | ||
| expect.stringContaining(' "sharp": false'), | ||
| "utf8", | ||
| ); | ||
| expect(execaMock).toHaveBeenCalledWith("pnpm", ["fix"], { | ||
| cwd: "/tmp/proofkit-regression/demo-browser", | ||
| stdio: "pipe", | ||
| }); | ||
| expect(execaMock).toHaveBeenCalledWith("pnpm", ["lint"], { | ||
| cwd: "/tmp/proofkit-regression/demo-browser", | ||
| stdio: "pipe", | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Regression case doesn’t actually lock pnpm v10 or verify “before install” ordering.
This test currently inherits execaMock version "9.0.0" from beforeEach, and it only checks presence of calls—not that pnpm-workspace.yaml is written before pnpm fix/lint.
Suggested tightening
it("writes pnpm build policy before install for pnpm 10", async () => {
mockState.appType = "webviewer";
+ execaMock.mockResolvedValue({ stdout: "10.27.0" });
await expect(
runInit("demo-webviewer", {
...browserFilemakerFlags,
noInstall: false,
dataSource: "none",
}),
).resolves.toBeUndefined();
expect(writeFileSyncMock).toHaveBeenCalledWith(
"/tmp/proofkit-regression/demo-browser/pnpm-workspace.yaml",
expect.stringContaining(' "sharp": false'),
"utf8",
);
expect(execaMock).toHaveBeenCalledWith("pnpm", ["fix"], {
cwd: "/tmp/proofkit-regression/demo-browser",
stdio: "pipe",
});
expect(execaMock).toHaveBeenCalledWith("pnpm", ["lint"], {
cwd: "/tmp/proofkit-regression/demo-browser",
stdio: "pipe",
});
+
+ const writeOrder = writeFileSyncMock.mock.invocationCallOrder.at(-1) ?? -1;
+ const fixCallIndex = execaMock.mock.calls.findIndex(
+ ([cmd, args]) => cmd === "pnpm" && args[0] === "fix",
+ );
+ const lintCallIndex = execaMock.mock.calls.findIndex(
+ ([cmd, args]) => cmd === "pnpm" && args[0] === "lint",
+ );
+ expect(writeOrder).toBeLessThan(execaMock.mock.invocationCallOrder[fixCallIndex] ?? Number.MAX_SAFE_INTEGER);
+ expect(writeOrder).toBeLessThan(execaMock.mock.invocationCallOrder[lintCallIndex] ?? Number.MAX_SAFE_INTEGER);
});🤖 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/tests/init-run-init-regression.test.ts` around lines 201 - 225,
The test "writes pnpm build policy before install for pnpm 10" currently relies
on the shared execaMock version and only asserts calls exist; update the test to
explicitly set the mocked pnpm version to "10.0.0" (override whatever shared
execaMock/version setup is used for other tests) and add an ordering assertion
that the writeFileSyncMock call that writes "pnpm-workspace.yaml" occurred
before the first pnpm execaMock invocation—e.g. set the mock pnpm version for
this test via the existing mock/version helper or by configuring execaMock to
return version "10.0.0", then assert
writeFileSyncMock.mock.invocationCallOrder[0] <
execaMock.mock.invocationCallOrder[0] (and keep the existing content/assertions
for the file contents and pnpm fix/lint calls).
- use `pnpm fix` for final init pass - keep setup going when fix fails - rename webviewer scaffold entry to `src/app.tsx`
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/tests/test-layer.ts`:
- Around line 349-353: The test may call Effect.fail with an undefined payload
because options.failures?.processRun is asserted rather than checked; extract
options.failures?.processRun into a local variable (e.g., const failure =
options.failures?.processRun) when processCommand matches
options.failProcessCommand, verify failure is defined and of type
ExternalCommandError, and if it's undefined return/throw a clear error (or
create a default ExternalCommandError) instead of calling
Effect.fail(undefined); update the branch that uses processCommand,
options.failProcessCommand, and Effect.fail to perform this guard and handle the
missing payload explicitly.
🪄 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: 293fe1be-e838-4dd5-a008-abe1b6520ee1
📒 Files selected for processing (5)
packages/cli/src/core/executeInitPlan.tspackages/cli/template/vite-wv/src/app.tsxpackages/cli/tests/executor.test.tspackages/cli/tests/init-scaffold-contract.test.tspackages/cli/tests/test-layer.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/template/vite-wv/src/app.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/tests/init-scaffold-contract.test.ts
- packages/cli/src/core/executeInitPlan.ts
Comment on lines
+349
to
+353
| const processCommand = [command, ...args].join(" "); | ||
| tracker?.commands.push(processCommand); | ||
| if (options.failProcessCommand === processCommand) { | ||
| return Effect.fail(options.failures?.processRun as ExternalCommandError); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
target = Path("packages/cli/tests/executor.test.ts")
src = target.read_text()
needle = "makeTestLayer({"
idx = 0
block_num = 0
while True:
start = src.find(needle, idx)
if start == -1:
break
block_num += 1
i = start + len("makeTestLayer(")
depth = 0
j = i
while j < len(src):
ch = src[j]
if ch == "{":
depth += 1
elif ch == "}":
depth -= 1
if depth == 0:
j += 1
break
j += 1
block = src[i:j]
has_fail_cmd = "failProcessCommand" in block
has_process_run = "processRun" in block
print(f"Block {block_num}: failProcessCommand={has_fail_cmd}, processRun={has_process_run}")
idx = j
PYRepository: proofsh/proofkit
Length of output: 737
🏁 Script executed:
cat -n packages/cli/tests/test-layer.ts | sed -n '345,360p'Repository: proofsh/proofkit
Length of output: 815
🏁 Script executed:
rg "makeTestLayer\(" --type ts -B2 -A10 packages/cli/tests/Repository: proofsh/proofkit
Length of output: 41887
🏁 Script executed:
cat -n packages/cli/tests/test-layer.ts | sed -n '1,50p'Repository: proofsh/proofkit
Length of output: 1784
🏁 Script executed:
rg "failures\?" packages/cli/tests/ -A 15 | head -100Repository: proofsh/proofkit
Length of output: 6921
🏁 Script executed:
cat -n packages/cli/tests/test-layer.ts | sed -n '70,115p'Repository: proofsh/proofkit
Length of output: 1754
🏁 Script executed:
rg "failProcessCommand" packages/cli/tests/ -B 5 -A 5Repository: proofsh/proofkit
Length of output: 2490
🏁 Script executed:
rg "Effect.fail.*processRun" packages/cli/tests/ -B 2 -A 2Repository: proofsh/proofkit
Length of output: 754
🏁 Script executed:
find packages/cli -name "*.ts" -type f ! -path "*/node_modules/*" ! -path "*/dist/*" -exec grep -l "ProcessService" {} \;Repository: proofsh/proofkit
Length of output: 201
🏁 Script executed:
cat -n packages/cli/tests/test-layer.ts | sed -n '60,72p'Repository: proofsh/proofkit
Length of output: 443
🏁 Script executed:
rg "ExternalCommandError" packages/cli/src/core/errors.ts -A 10 | head -30Repository: proofsh/proofkit
Length of output: 850
Guard against missing error payload when failProcessCommand is targeted.
Line 352 uses a type assertion to cast options.failures?.processRun as ExternalCommandError, but optional chaining with undefined combined with as allows undefined to reach Effect.fail(). This bypasses type safety and weakens test determinism.
Extract the value to a variable and guard against undefined before use:
Suggested fix
run: (command: string, args: string[]) => {
const processCommand = [command, ...args].join(" ");
tracker?.commands.push(processCommand);
+ const processRunFailure = options.failures?.processRun;
if (options.failProcessCommand === processCommand) {
- return Effect.fail(options.failures?.processRun as ExternalCommandError);
+ if (!processRunFailure) {
+ throw new Error("makeTestLayer requires failures.processRun when failProcessCommand is set.");
+ }
+ return Effect.fail(processRunFailure as ExternalCommandError);
}
if (!options.failProcessCommand && options.failures?.processRun) {
return Effect.fail(options.failures.processRun as ExternalCommandError);
}
return Effect.succeed({ stdout: "", stderr: "" });
},📝 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
| const processCommand = [command, ...args].join(" "); | |
| tracker?.commands.push(processCommand); | |
| if (options.failProcessCommand === processCommand) { | |
| return Effect.fail(options.failures?.processRun as ExternalCommandError); | |
| } | |
| const processCommand = [command, ...args].join(" "); | |
| tracker?.commands.push(processCommand); | |
| const processRunFailure = options.failures?.processRun; | |
| if (options.failProcessCommand === processCommand) { | |
| if (!processRunFailure) { | |
| throw new Error("makeTestLayer requires failures.processRun when failProcessCommand is set."); | |
| } | |
| return Effect.fail(processRunFailure as ExternalCommandError); | |
| } |
🤖 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/tests/test-layer.ts` around lines 349 - 353, The test may call
Effect.fail with an undefined payload because options.failures?.processRun is
asserted rather than checked; extract options.failures?.processRun into a local
variable (e.g., const failure = options.failures?.processRun) when
processCommand matches options.failProcessCommand, verify failure is defined and
of type ExternalCommandError, and if it's undefined return/throw a clear error
(or create a default ExternalCommandError) instead of calling
Effect.fail(undefined); update the branch that uses processCommand,
options.failProcessCommand, and Effect.fail to perform this guard and handle the
missing payload explicitly.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/core/executeInitPlan.ts`:
- Around line 439-447: The runFix branch currently calls getPackageScriptCommand
and yields Effect.either(processService.run(...)) but ignores the Either result,
swallowing failures; change this so the Either is inspected and any Left
(process failure) is propagated as a failing effect (or turned into a thrown
error) so init does not report success on a broken fix step—i.e., after calling
processService.run from executeInitPlan.ts (the runFix branch that references
plan.tasks.runFix, getPackageScriptCommand, and processService.run) handle the
Either return value and return/fail the surrounding Effect when the process
failed instead of discarding it.
- Around line 450-460: The block guarded by plan.tasks.runLint currently calls
getPackageScriptCommand(plan, "fix") then runs that command; change it to
resolve and run the lint script instead by calling getPackageScriptCommand(plan,
"lint") so the lint task actually executes (the surrounding symbols are
plan.tasks.runLint, getPackageScriptCommand, and processService.run); update the
variable name (e.g., fixCommand → lintCommand) and the subsequent
processService.run invocation to use the lint command and args so lint runs
rather than fix being executed twice.
🪄 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: fb8decb7-a96d-4c2d-b72a-df6697df7a00
📒 Files selected for processing (2)
packages/cli/src/core/executeInitPlan.tspackages/cli/tests/executor.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cli/tests/executor.test.ts
Comment on lines
+439
to
+447
| if (plan.tasks.runFix) { | ||
| const fixCommand = getPackageScriptCommand(plan, "fix"); | ||
| yield* Effect.either( | ||
| processService.run(fixCommand.command, fixCommand.args, { | ||
| cwd: plan.targetDir, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }), | ||
| ); |
There was a problem hiding this comment.
Don't swallow runFix failures.
Line 441 converts the command failure into an Either, but the result is ignored. That makes a broken fix step invisible and still lets init report success.
Suggested fix
if (plan.tasks.runFix) {
const fixCommand = getPackageScriptCommand(plan, "fix");
- yield* Effect.either(
- processService.run(fixCommand.command, fixCommand.args, {
- cwd: plan.targetDir,
- stdout: "pipe",
- stderr: "pipe",
- }),
- );
+ yield* processService.run(fixCommand.command, fixCommand.args, {
+ cwd: plan.targetDir,
+ stdout: "pipe",
+ stderr: "pipe",
+ });
}🤖 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/core/executeInitPlan.ts` around lines 439 - 447, The runFix
branch currently calls getPackageScriptCommand and yields
Effect.either(processService.run(...)) but ignores the Either result, swallowing
failures; change this so the Either is inspected and any Left (process failure)
is propagated as a failing effect (or turned into a thrown error) so init does
not report success on a broken fix step—i.e., after calling processService.run
from executeInitPlan.ts (the runFix branch that references plan.tasks.runFix,
getPackageScriptCommand, and processService.run) handle the Either return value
and return/fail the surrounding Effect when the process failed instead of
discarding it.
Comment on lines
+450
to
+460
| if (plan.tasks.runLint) { | ||
| const fixCommand = getPackageScriptCommand(plan, "fix"); | ||
| const result = yield* Effect.either( | ||
| processService.run(fixCommand.command, fixCommand.args, { | ||
| cwd: plan.targetDir, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }), | ||
| ); | ||
| if (result._tag === "Left") { | ||
| consoleService.warn("Lint fix did not succeed; continuing setup."); |
There was a problem hiding this comment.
Run the lint script here, not fix.
Line 451 resolves "fix" again, so this block never executes the lint script. In the common case where both tasks are enabled, setup runs fix twice and skips lint entirely.
Suggested fix
if (plan.tasks.runLint) {
- const fixCommand = getPackageScriptCommand(plan, "fix");
+ const lintCommand = getPackageScriptCommand(plan, "lint");
const result = yield* Effect.either(
- processService.run(fixCommand.command, fixCommand.args, {
+ processService.run(lintCommand.command, lintCommand.args, {
cwd: plan.targetDir,
stdout: "pipe",
stderr: "pipe",
}),
);
if (result._tag === "Left") {
- consoleService.warn("Lint fix did not succeed; continuing setup.");
+ consoleService.warn("Lint did not succeed; continuing setup.");
}
}📝 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
| if (plan.tasks.runLint) { | |
| const fixCommand = getPackageScriptCommand(plan, "fix"); | |
| const result = yield* Effect.either( | |
| processService.run(fixCommand.command, fixCommand.args, { | |
| cwd: plan.targetDir, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }), | |
| ); | |
| if (result._tag === "Left") { | |
| consoleService.warn("Lint fix did not succeed; continuing setup."); | |
| if (plan.tasks.runLint) { | |
| const lintCommand = getPackageScriptCommand(plan, "lint"); | |
| const result = yield* Effect.either( | |
| processService.run(lintCommand.command, lintCommand.args, { | |
| cwd: plan.targetDir, | |
| stdout: "pipe", | |
| stderr: "pipe", | |
| }), | |
| ); | |
| if (result._tag === "Left") { | |
| consoleService.warn("Lint did not succeed; continuing setup."); | |
| } | |
| } |
🤖 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/core/executeInitPlan.ts` around lines 450 - 460, The block
guarded by plan.tasks.runLint currently calls getPackageScriptCommand(plan,
"fix") then runs that command; change it to resolve and run the lint script
instead by calling getPackageScriptCommand(plan, "lint") so the lint task
actually executes (the surrounding symbols are plan.tasks.runLint,
getPackageScriptCommand, and processService.run); update the variable name
(e.g., fixCommand → lintCommand) and the subsequent processService.run
invocation to use the lint command and args so lint runs rather than fix being
executed twice.
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
Testing
pnpm run ciSummary by CodeRabbit
New Features
Dependencies