Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: fe43be6 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 ignored due to path filters (2)
📒 Files selected for processing (39)
💤 Files with no reviewable changes (31)
✅ Files skipped from review due to trivial changes (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "ProofKit CLI (init)"
participant NameUtils as "parseNameAndPath/validateAppName"
participant TemplateSvc as "TemplateService"
participant Scaffold as "scaffoldProject"
participant FS as "Filesystem"
User->>CLI: run `proofkit init [dir]` (no --ui)
CLI->>NameUtils: normalize/validate project name (whitespace→dash, lowercased, handle ".")
NameUtils-->>CLI: normalized name + path
CLI->>TemplateSvc: getTemplateDir(appType, _ui) -- ui ignored
TemplateSvc-->>CLI: template path (e.g., nextjs-shadcn)
CLI->>Scaffold: scaffold chosen template into targetDir
Scaffold->>FS: write scaffold files (including `CLAUDE.md`)
Scaffold->>FS: write `.cursorignore` with content `CLAUDE.md\n`
FS-->>Scaffold: write confirmations
Scaffold-->>CLI: scaffold complete
CLI-->>User: success (created project)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/utils/parseNameAndPath.ts (1)
30-33:⚠️ Potential issue | 🟠 MajorNormalize cwd-derived
appNameto match input normalization and validation.When
appName === ".", the code derives it fromprocess.cwd()basename without normalizing. However, the initial input is normalized at line 24 (.replace(whitespaceRegex, "-").toLowerCase()), andvalidateAppNamealso normalizes cwd-derived names before validation. This creates an inconsistency: if the directory is/tmp/My App, validation passes (normalizes tomy-app), butparseNameAndPathreturns"My App"unnormalized, which then gets written topackage.jsonas an invalid package name (spaces violate the valid name regex).Normalize the cwd-derived
appNameto match the initial input normalization:Suggested fix
if (appName === ".") { const parsedCwd = pathModule.resolve(process.cwd()); appName = pathModule.basename(parsedCwd); appName = appName.replace(whitespaceRegex, "-").toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils/parseNameAndPath.ts` around lines 30 - 33, When appName is set from the cwd in parseNameAndPath (the block checking if appName === "."), normalize it using the same logic as the initial input normalization so it matches validateAppName; specifically, after deriving appName from pathModule.basename(process.cwd()), run appName = appName.replace(whitespaceRegex, "-").toLowerCase() so the cwd-derived name is hyphenized and lowercased before being returned or written to package.json.packages/cli/src/utils/validateAppName.ts (1)
19-21:⚠️ Potential issue | 🟠 MajorThe
"."check bypasses validation.When
input === ".", the function returnsundefined(valid) without actually validating that the current working directory name conforms tovalidationRegExp. This differs fromprojectName.ts(lines 41-46), which normalizes and validates the current directory name when the input is".".If the user runs the CLI in a directory with an invalid name (e.g., containing uppercase letters or special characters), this version would incorrectly report it as valid.
🐛 Proposed fix to validate the current directory name
- if (input === "." || validationRegExp.test(appName ?? "")) { - return; + if (input === ".") { + const path = await import("node:path"); + const currentDirName = path.basename(path.resolve(process.cwd())); + const normalizedDir = currentDirName.replace(whitespaceRegex, "-").toLowerCase(); + return validationRegExp.test(normalizedDir) + ? undefined + : "Name must consist of only lowercase alphanumeric characters, '-', and '_'"; + } + + if (validationRegExp.test(appName ?? "")) { + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils/validateAppName.ts` around lines 19 - 21, The current early-return when input === "." bypasses validation; in validateAppName replace that branch so the code derives the current directory name (e.g., appName = path.basename(process.cwd()) or same logic used in projectName.ts) and then run validationRegExp.test(appName) instead of returning; update validateAppName to normalize/compute appName for "." inputs, ensure validationRegExp is used against that computed appName, and add an import for path if necessary so the behavior matches projectName.ts's handling of ".".packages/cli/src/utils/projectName.ts (1)
25-27:⚠️ Potential issue | 🟡 MinorInconsistent normalization for
"."case betweenparseNameAndPathandvalidateAppName.When the input resolves to
".",parseNameAndPathuses the rawpath.basename(process.cwd())without applying whitespace-to-dash normalization or lowercasing (line 26). However,validateAppName(lines 43) does normalize the current directory name before validation.This could lead to a mismatch: the project might be created with an unnormalized directory name (e.g.,
"My App") while validation expects the normalized form.♻️ Proposed fix to normalize the cwd basename consistently
if (scopedAppName === ".") { - scopedAppName = path.basename(path.resolve(process.cwd())); + scopedAppName = path.basename(path.resolve(process.cwd())).replace(WHITESPACE_REGEX, "-").toLowerCase(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils/projectName.ts` around lines 25 - 27, parseNameAndPath sets scopedAppName to path.basename(path.resolve(process.cwd())) when the input is "." but doesn't apply the same whitespace-to-dash normalization and lowercasing that validateAppName expects, causing inconsistent names; update parseNameAndPath so that when scopedAppName === "." it computes the cwd basename and then runs the same normalization routine (convert whitespace to dashes and lowercase) before returning/using scopedAppName (ensure you reuse or mirror the same normalization logic used in validateAppName to normalize the cwd basename).
🧹 Nitpick comments (1)
packages/cli/src/utils/parseNameAndPath.ts (1)
36-39: RedundantfindIndexcall.
indexOfDelimiteris already computed on line 36, but line 37 callsfindIndexagain with the same predicate. UseindexOfDelimiterin the condition instead.♻️ Proposed fix
// If the first part is a @, it's a scoped package const indexOfDelimiter = paths.findIndex((p) => p.startsWith("@")); - if (paths.findIndex((p) => p.startsWith("@")) !== -1) { + if (indexOfDelimiter !== -1) { appName = paths.slice(indexOfDelimiter).join("/"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/utils/parseNameAndPath.ts` around lines 36 - 39, The code computes indexOfDelimiter using paths.findIndex but then redundantly calls paths.findIndex again in the if condition; replace the second findIndex call with the computed indexOfDelimiter (i.e. use if (indexOfDelimiter !== -1) { appName = paths.slice(indexOfDelimiter).join("/"); }) so the function parseNameAndPath uses the existing indexOfDelimiter variable rather than calling findIndex twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/utils/parseNameAndPath.ts`:
- Around line 30-33: When appName is set from the cwd in parseNameAndPath (the
block checking if appName === "."), normalize it using the same logic as the
initial input normalization so it matches validateAppName; specifically, after
deriving appName from pathModule.basename(process.cwd()), run appName =
appName.replace(whitespaceRegex, "-").toLowerCase() so the cwd-derived name is
hyphenized and lowercased before being returned or written to package.json.
In `@packages/cli/src/utils/projectName.ts`:
- Around line 25-27: parseNameAndPath sets scopedAppName to
path.basename(path.resolve(process.cwd())) when the input is "." but doesn't
apply the same whitespace-to-dash normalization and lowercasing that
validateAppName expects, causing inconsistent names; update parseNameAndPath so
that when scopedAppName === "." it computes the cwd basename and then runs the
same normalization routine (convert whitespace to dashes and lowercase) before
returning/using scopedAppName (ensure you reuse or mirror the same normalization
logic used in validateAppName to normalize the cwd basename).
In `@packages/cli/src/utils/validateAppName.ts`:
- Around line 19-21: The current early-return when input === "." bypasses
validation; in validateAppName replace that branch so the code derives the
current directory name (e.g., appName = path.basename(process.cwd()) or same
logic used in projectName.ts) and then run validationRegExp.test(appName)
instead of returning; update validateAppName to normalize/compute appName for
"." inputs, ensure validationRegExp is used against that computed appName, and
add an import for path if necessary so the behavior matches projectName.ts's
handling of ".".
---
Nitpick comments:
In `@packages/cli/src/utils/parseNameAndPath.ts`:
- Around line 36-39: The code computes indexOfDelimiter using paths.findIndex
but then redundantly calls paths.findIndex again in the if condition; replace
the second findIndex call with the computed indexOfDelimiter (i.e. use if
(indexOfDelimiter !== -1) { appName = paths.slice(indexOfDelimiter).join("/");
}) so the function parseNameAndPath uses the existing indexOfDelimiter variable
rather than calling findIndex twice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 730839a9-084a-42cd-9135-000f8025f990
📒 Files selected for processing (11)
.changeset/remove-cli-ui-flag.md.changeset/soften-project-name-spaces.md.changeset/tidy-dots-speak.mdpackages/cli/src/cli/init.tspackages/cli/src/index.tspackages/cli/src/utils/parseNameAndPath.tspackages/cli/src/utils/projectName.tspackages/cli/src/utils/validateAppName.tspackages/cli/tests/cli.test.tspackages/cli/tests/init-non-interactive-failures.test.tspackages/cli/tests/project-name.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/cli/tests/integration.test.ts (1)
77-78: De-duplicate pointer-content literals in this suiteConsider constants for
@AGENTS.mdand.cursorignorecontent to avoid future drift between test blocks.As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".
Also applies to: 204-205
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/integration.test.ts` around lines 77 - 78, Extract the literal expected strings "@AGENTS.md\n" and "CLAUDE.md\n" into descriptively named constants (e.g., EXPECTED_AGENTS_POINTER_CONTENT and EXPECTED_CURSORIGNORE_CONTENT) near the top of the test suite and replace occurrences used in the assertions for claudeFile and cursorIgnoreFile (and the other occurrences at the second block around the previously noted lines) so all tests reference the same constants instead of duplicating magic literals.packages/cli/tests/init-fixtures.ts (1)
34-55: Type the exported scaffold-artifact contract explicitlySince this helper is shared across tests and keeps expanding, an explicit return type will prevent silent contract drift.
As per coding guidelines, "Use explicit types for function parameters and return values when they enhance clarity".♻️ Proposed patch
+interface ScaffoldArtifacts { + packageJson: Record<string, unknown>; + proofkitJson: Record<string, unknown>; + envFile: string; + typegenConfig: string | undefined; + agentsFile: string | undefined; + claudeFile: string | undefined; + cursorIgnoreFile: string | undefined; + launchConfig: string | undefined; +} + -export async function readScaffoldArtifacts(projectDir: string) { +export async function readScaffoldArtifacts(projectDir: string): Promise<ScaffoldArtifacts> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/init-fixtures.ts` around lines 34 - 55, Add an explicit exported return type for readScaffoldArtifacts to lock the scaffold-artifact contract: define and export an interface (e.g., ReadScaffoldArtifactsResult) listing each property (packageJson, proofkitJson, envFile, typegenConfig, agentsFile, claudeFile, cursorIgnoreFile, launchConfig) with their correct types (strings or undefined where pathExists makes them optional, and the JSON types for packageJson/proofkitJson), then annotate the function signature as export async function readScaffoldArtifacts(projectDir: string): Promise<ReadScaffoldArtifactsResult> and ensure the returned object matches the interface.packages/cli/tests/init-scaffold-contract.test.ts (1)
151-153: Extract repeated agent-pointer literals into constants
"@AGENTS.md\n"and"CLAUDE.md\n"are duplicated across cases; constants make intent clearer and reduce drift.As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names".🧹 Proposed patch
const ansiStylePrefixPattern = /^[0-9;]*m/; +const claudePointerContent = "@AGENTS.md\n" as const; +const cursorIgnoreContent = "CLAUDE.md\n" as const; @@ - expect(readFileSync(join(browserProjectDir, "CLAUDE.md"), "utf-8")).toBe("@AGENTS.md\n"); - expect(readFileSync(join(browserProjectDir, ".cursorignore"), "utf-8")).toBe("CLAUDE.md\n"); + expect(readFileSync(join(browserProjectDir, "CLAUDE.md"), "utf-8")).toBe(claudePointerContent); + expect(readFileSync(join(browserProjectDir, ".cursorignore"), "utf-8")).toBe(cursorIgnoreContent); @@ - expect(readFileSync(join(webviewerProjectDir, "CLAUDE.md"), "utf-8")).toBe("@AGENTS.md\n"); - expect(readFileSync(join(webviewerProjectDir, ".cursorignore"), "utf-8")).toBe("CLAUDE.md\n"); + expect(readFileSync(join(webviewerProjectDir, "CLAUDE.md"), "utf-8")).toBe(claudePointerContent); + expect(readFileSync(join(webviewerProjectDir, ".cursorignore"), "utf-8")).toBe(cursorIgnoreContent);Also applies to: 192-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/tests/init-scaffold-contract.test.ts` around lines 151 - 153, Replace repeated string literals with named constants: define e.g. const AGENTS_POINTER = "@AGENTS.md\n" and const CLAUDE_POINTER = "CLAUDE.md\n" at the top of the test file, then use those constants in the expect assertions that call readFileSync(join(browserProjectDir, "CLAUDE.md"), ...) and readFileSync(join(browserProjectDir, ".cursorignore"), ...), and in any other occurrences (including the other cases noted) so the expectations reference AGENTS_POINTER and CLAUDE_POINTER instead of raw string literals.
🤖 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/helpers/scaffoldProject.ts`:
- Line 132: The current call in scaffoldProject that unconditionally writes
".cursorignore" (fs.writeFileSync(path.join(projectDir, ".cursorignore"),
"CLAUDE.md\n", "utf8")) will clobber any existing ignores; change it to merge
safely by checking for an existing file (fs.existsSync / fs.readFileSync),
reading its contents, appending "CLAUDE.md" only if not already present (ensure
newline normalization and deduplication), and then write the merged content back
with fs.writeFileSync; reference scaffoldProject and the
projectDir/.cursorignore handling when implementing the read-merge-write
behavior.
---
Nitpick comments:
In `@packages/cli/tests/init-fixtures.ts`:
- Around line 34-55: Add an explicit exported return type for
readScaffoldArtifacts to lock the scaffold-artifact contract: define and export
an interface (e.g., ReadScaffoldArtifactsResult) listing each property
(packageJson, proofkitJson, envFile, typegenConfig, agentsFile, claudeFile,
cursorIgnoreFile, launchConfig) with their correct types (strings or undefined
where pathExists makes them optional, and the JSON types for
packageJson/proofkitJson), then annotate the function signature as export async
function readScaffoldArtifacts(projectDir: string):
Promise<ReadScaffoldArtifactsResult> and ensure the returned object matches the
interface.
In `@packages/cli/tests/init-scaffold-contract.test.ts`:
- Around line 151-153: Replace repeated string literals with named constants:
define e.g. const AGENTS_POINTER = "@AGENTS.md\n" and const CLAUDE_POINTER =
"CLAUDE.md\n" at the top of the test file, then use those constants in the
expect assertions that call readFileSync(join(browserProjectDir, "CLAUDE.md"),
...) and readFileSync(join(browserProjectDir, ".cursorignore"), ...), and in any
other occurrences (including the other cases noted) so the expectations
reference AGENTS_POINTER and CLAUDE_POINTER instead of raw string literals.
In `@packages/cli/tests/integration.test.ts`:
- Around line 77-78: Extract the literal expected strings "@AGENTS.md\n" and
"CLAUDE.md\n" into descriptively named constants (e.g.,
EXPECTED_AGENTS_POINTER_CONTENT and EXPECTED_CURSORIGNORE_CONTENT) near the top
of the test suite and replace occurrences used in the assertions for claudeFile
and cursorIgnoreFile (and the other occurrences at the second block around the
previously noted lines) so all tests reference the same constants instead of
duplicating magic literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f32be084-2253-4658-8560-8f33c5c72939
📒 Files selected for processing (13)
.changeset/cli-init-claude-cursorignore.mdapps/docs/src/app/layout.tsxpackages/cli/src/core/planInit.tspackages/cli/src/helpers/scaffoldProject.tspackages/cli/template/nextjs-mantine/CLAUDE.mdpackages/cli/template/nextjs-mantine/CLAUDE.mdpackages/cli/template/nextjs-shadcn/CLAUDE.mdpackages/cli/template/nextjs-shadcn/CLAUDE.mdpackages/cli/template/vite-wv/CLAUDE.mdpackages/cli/template/vite-wv/CLAUDE.mdpackages/cli/tests/init-fixtures.tspackages/cli/tests/init-scaffold-contract.test.tspackages/cli/tests/integration.test.ts
✅ Files skipped from review due to trivial changes (9)
- packages/cli/template/vite-wv/CLAUDE.md
- packages/cli/template/vite-wv/CLAUDE.md
- packages/cli/template/nextjs-shadcn/CLAUDE.md
- apps/docs/src/app/layout.tsx
- .changeset/cli-init-claude-cursorignore.md
- packages/cli/template/nextjs-mantine/CLAUDE.md
- packages/cli/template/nextjs-mantine/CLAUDE.md
- packages/cli/template/nextjs-shadcn/CLAUDE.md
- packages/cli/src/core/planInit.ts
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.
--uifrom init helpChecks:
pnpm --filter @proofkit/cli typecheckpasspnpm --filter @proofkit/cli test -- --run tests/init-non-interactive-failures.test.ts tests/project-name.test.tspasspnpm run cifails in@proofkit/registryon sandboxedtsxIPC listenEPERMSummary by CodeRabbit
Breaking Changes
--uiCLI flag; init now always scaffolds the shadcn UI.New Features
Documentation
.during init refers to the current directory.Tests
--uihelp, and generated files.