Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Collaborator
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (62)
📝 WalkthroughWalkthroughAdds a new scaffold CLI package ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "proofkit-new CLI"
participant Prompt as "PromptService (local)"
participant Planner as "resolveInitRequest / planInit"
participant FileMaker as "FileMakerService"
participant Exec as "executeInitPlan"
User->>CLI: run init
CLI->>Prompt: ask project name & flags
Prompt-->>CLI: responses
CLI->>Planner: build InitRequest / InitPlan
Planner-->>CLI: InitPlan
alt FileMaker selected
CLI->>FileMaker: validate server, list files & keys
FileMaker-->>CLI: server status, files, keys
CLI->>Prompt: select file / key
Prompt-->>CLI: selection
CLI->>FileMaker: create bootstrap artifacts
FileMaker-->>CLI: artifacts
end
CLI->>Exec: execute InitPlan
Exec->>Exec: prepare dir, copy template, write files, update env/typegen
alt install/codegen/git enabled
Exec->>Exec: run install / codegen / git init
end
Exec-->>CLI: success
CLI-->>User: print next steps
sequenceDiagram
participant Caller as "Existing CLI module"
participant PromptsLayer as "local prompts module"
participant Clack as "@clack/prompts"
participant Inquirer as "@inquirer/prompts"
Caller->>PromptsLayer: import prompt helpers
PromptsLayer->>Clack: re-export intro/outro/log/spinner/cancel
PromptsLayer->>Inquirer: delegate select/search/multi-select calls
PromptsLayer->>PromptsLayer: normalize options, handle cancel sentinel, filter by keywords
Inquirer-->>PromptsLayer: user selection / cancel
PromptsLayer-->>Caller: return value or CANCEL_SYMBOL
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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.
Actionable comments posted: 9
🧹 Nitpick comments (29)
packages/new/src/utils/browserOpen.ts (1)
3-9: Return an explicit success signal fromopenBrowser.On Line 3, the inferred
Promise<void>plus the silentcatchon Lines 6-8 gives callers no way to react when opening fails. Returning a boolean keeps behavior simple and makes fallback UX explicit.♻️ Proposed refactor
-export async function openBrowser(url: string) { +export async function openBrowser(url: string): Promise<boolean> { try { await open(url); + return true; } catch { // Ignore open failures and let the user copy the URL manually. + return false; } }As per coding guidelines
**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity, and**/*.{js,jsx,ts,tsx}: Handle errors appropriately in async code with try-catch blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/browserOpen.ts` around lines 3 - 9, The openBrowser function currently swallows errors and returns void, so callers cannot know if opening succeeded; change export async function openBrowser(url: string) to explicitly return Promise<boolean>, await open(url) inside a try and return true on success, catch errors and return false (optionally log the error) so callers get a clear success/failure signal from openBrowser.packages/new/src/utils/http.ts (1)
32-40: TightendeleteJsontyping to avoid implicitany.
deleteJsoncurrently infersAxiosResponse<any>. Prefer a typed generic withunknowndefault for safer API contracts.🔧 Proposed refactor
-import axios from "axios"; +import axios, { type AxiosResponse } from "axios"; @@ -export async function deleteJson(url: string, options?: { headers?: Record<string, string>; timeout?: number }) { - const response = await axios.delete(url, { +export async function deleteJson<T = unknown>( + url: string, + options?: { headers?: Record<string, string>; timeout?: number }, +): Promise<AxiosResponse<T>> { + const response = await axios.delete<T>(url, { headers: options?.headers, httpsAgent, timeout: options?.timeout ?? 10_000, validateStatus: null, }); return response; }As per coding guidelines, "Prefer
unknownoveranywhen the type is genuinely unknown" and "**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/http.ts` around lines 32 - 40, The deleteJson function should be made generic to avoid AxiosResponse<any>; change its signature to a generic like deleteJson<T = unknown>(url: string, options?: { headers?: Record<string,string>; timeout?: number }): Promise<AxiosResponse<T>> and use axios.delete<T>(...) so the returned type is AxiosResponse<T> (defaulting to unknown) instead of any; update the import/type references if needed to include AxiosResponse and ensure axios.delete is called with the generic type parameter.packages/new/src/utils/renderTitle.ts (1)
15-17: Consider adding a clarifying comment for the yarn/pnpm spacing logic.The conditional empty line for yarn/pnpm appears intentional but the reason isn't obvious. A brief comment would help future maintainers understand this behavior.
📝 Suggested documentation
export function renderTitle(version = "0.0.0-private") { const packageManager = detectUserPackageManager(); + // yarn and pnpm output additional info before the script runs; add spacing for visual separation if (packageManager === "yarn" || packageManager === "pnpm") { console.log(""); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/renderTitle.ts` around lines 15 - 17, The conditional console.log("") in renderTitle.ts that runs when packageManager === "yarn" || packageManager === "pnpm" is unclear; add a short clarifying comment directly above that if explaining why an extra blank line is required for yarn/pnpm output formatting (mentioning any specific CLI/UX reason, e.g., alignment with subsequent output or spacing required by those package managers). Locate the check using the packageManager variable in the renderTitle.ts function and insert a one-line comment describing the intent so future maintainers understand the spacing logic.packages/new/src/utils/packageManager.ts (1)
16-19: Inconsistent default package manager fallback.When
npm_config_user_agentis set but doesn't match known patterns, the function returns"npm"(line 16). However, when the env var is undefined, it returns"pnpm"(line 19).If
pnpmis the intended project default, consider using it consistently:♻️ Suggested consistency fix
} - return "npm"; + return "pnpm"; } return "pnpm";Alternatively, if
npmshould be the fallback for unknown agents, update line 19 to return"npm"for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/packageManager.ts` around lines 16 - 19, The function that inspects npm_config_user_agent returns different fallbacks ("npm" when the env var exists but is unrecognized, and "pnpm" when it's undefined); pick a single consistent default and update both return sites accordingly—either change the earlier return "npm" to "pnpm" so unknown agents also fall back to pnpm, or change the later return "pnpm" to "npm" so undefined env falls back to npm; search for the npm_config_user_agent check and the two return "npm"/return "pnpm" statements and make them consistent.packages/cli/src/cli/prompts.ts (1)
82-188: Make the cancel-aware return type explicit on the exported prompt helpers.These helpers all return
Promise<... | symbol>throughwithCancelSentinel(). Writing that contract out at the API boundary makes the cancellation requirement much easier to spot and review at call sites.As per coding guidelines, `**/*.{ts,tsx}`: Use explicit types for function parameters and return values when they enhance clarity.✍️ Proposed refactor
export function text(options: { message: string; defaultValue?: string; placeholder?: string; validate?: (value: string) => string | undefined; -}) { +}): Promise<string | symbol> { @@ -export function password(options: { message: string; validate?: (value: string) => string | undefined }) { +export function password(options: { + message: string; + validate?: (value: string) => string | undefined; +}): Promise<string | symbol> { @@ -export function confirm(options: { message: string; initialValue?: boolean; active?: string; inactive?: string }) { +export function confirm(options: { + message: string; + initialValue?: boolean; + active?: string; + inactive?: string; +}): Promise<boolean | symbol> { @@ export function select<T extends string>(options: { message: string; options: PromptOption<T>[]; maxItems?: number; initialValue?: T; -}) { +}): Promise<T | symbol> { @@ export function searchSelect<T extends string>(options: { message: string; searchLabel?: string; emptyMessage?: string; options: SearchPromptOption<T>[]; -}) { +}): Promise<T | symbol> { @@ export function multiSearchSelect<T extends string>(options: { message: string; options: SearchPromptOption<T>[]; required?: boolean; -}) { +}): Promise<T[] | symbol> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/prompts.ts` around lines 82 - 188, Each exported prompt helper should have an explicit return type that reflects withCancelSentinel's Promise<... | symbol> contract; update the signatures for text and password to return Promise<string | symbol>, confirm to return Promise<boolean | symbol>, select and searchSelect to return Promise<T | symbol>, and multiSearchSelect to return Promise<T[] | symbol>, keeping existing generics (T) and leaving implementations unchanged so callers see the cancel-aware type at the API boundary.packages/new/tests/init-fixtures.ts (1)
30-47: Add explicit return types to exported test helpers.Line 30 and Line 34 would benefit from explicit return types to make fixture contracts stable and prevent implicit type widening.
♻️ Suggested change
+type SharedTemplateName = "nextjs-shadcn" | "nextjs-mantine" | "vite-wv"; + +interface ScaffoldArtifacts { + packageJson: unknown; + proofkitJson: unknown; + envFile: string; + typegenConfig?: string; +} + -export function getSharedTemplateDir(templateName: "nextjs-shadcn" | "nextjs-mantine" | "vite-wv") { +export function getSharedTemplateDir(templateName: SharedTemplateName): string { return path.resolve(__dirname, `../../cli/template/${templateName}`); } -export async function readScaffoldArtifacts(projectDir: string) { +export async function readScaffoldArtifacts(projectDir: string): Promise<ScaffoldArtifacts> { const packageJson = await fs.readJson(path.join(projectDir, "package.json")); const proofkitJson = await fs.readJson(path.join(projectDir, "proofkit.json")); const envFile = await fs.readFile(path.join(projectDir, ".env"), "utf8");As per coding guidelines, "Use explicit types for function parameters and return values when they enhance clarity" and "Prefer
unknownoveranywhen the type is genuinely unknown."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/init-fixtures.ts` around lines 30 - 47, Add explicit return types to the exported helpers: annotate getSharedTemplateDir to return string, and annotate readScaffoldArtifacts to return Promise<{ packageJson: unknown; proofkitJson: unknown; envFile: string; typegenConfig?: string }>. Keep the existing parameter types (templateName and projectDir) and use unknown for JSON payloads instead of any to follow the guideline.packages/cli/src/cli/react-email.ts (1)
2-2: Prefer named imports for prompts utilities.Line 2 uses a namespace import for
~/cli/prompts.js; use specific imports to keep call sites explicit.♻️ Suggested change
-import * as p from "~/cli/prompts.js"; +import { spinner } from "~/cli/prompts.js"; @@ - const spinner = p.spinner(); - spinner.start("Adding React Email"); + const addSpinner = spinner(); + addSpinner.start("Adding React Email"); @@ - spinner.stop("React Email added"); + addSpinner.stop("React Email added");As per coding guidelines, "Prefer specific imports over namespace imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/react-email.ts` at line 2, The file currently uses a namespace import "import * as p from '~/cli/prompts.js';" — replace this with explicit named imports from '~/cli/prompts.js' by scanning for usages of the "p." namespace (e.g., p.promptName, p.confirm, p.select, etc.) and importing only those identifiers (e.g., import { confirm, input, select } from '~/cli/prompts.js';). Remove the namespace import and update call sites to use the imported names directly to keep call sites explicit and adhere to the "prefer specific imports" guideline.packages/cli/src/cli/ottofms.ts (1)
7-7: Prefer named imports from the prompts module.Line 7 uses a namespace import; named imports would make prompt API usage clearer and consistent with project conventions.
As per coding guidelines, "Prefer specific imports over namespace imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/ottofms.ts` at line 7, Replace the namespace import "import * as clack from '~/cli/prompts.js'" with explicit named imports from the prompts module for the specific prompt functions used in this file (for example confirm, text, select, etc.), update all call sites that currently use the clack.<fn> form to call the imported identifiers directly, and ensure the import list matches the actual symbols referenced in ottofms.ts to satisfy the project guideline preferring specific imports over namespace imports.packages/cli/src/cli/add/data-source/deploy-demo-file.ts (1)
2-2: Use a specific prompts import here as well.Line 2 currently uses a namespace import; this should be narrowed to the specific prompt utilities used in this file.
♻️ Suggested change
-import * as p from "~/cli/prompts.js"; +import { spinner } from "~/cli/prompts.js"; @@ - const spinner = p.spinner(); - spinner.start("Deploying ProofKit Demo file..."); + const deploySpinner = spinner(); + deploySpinner.start("Deploying ProofKit Demo file..."); @@ - spinner.stop(); + deploySpinner.stop();As per coding guidelines, "Prefer specific imports over namespace imports."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/add/data-source/deploy-demo-file.ts` at line 2, Replace the namespace import "import * as p from \"~/cli/prompts.js\";" with explicit named imports for only the prompt utilities this file uses: find all references to p.<fnName> in deploy-demo-file.ts (e.g., p.confirm, p.input, p.select or similar) and change the import to "import { confirm, input, select } from '~/cli/prompts.js';" (using the exact function names found). Ensure there are no remaining "p." usages and update any call sites accordingly so the file uses the specific imported symbols instead of the namespace import.packages/cli/src/cli/remove/index.ts (1)
2-2: Use a named prompt import instead of namespace import.This file only needs
select, so a direct named import is preferable.As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/remove/index.ts` at line 2, Replace the namespace import of prompts with a named import for the select function: remove "import * as p from '~/cli/prompts.js';" and import select directly from "~/cli/prompts.js", then update usages of p.select to call select instead; this keeps the file using the specific symbol (select) rather than the namespace (p).packages/cli/src/installers/proofkit-auth.ts (1)
9-9: Prefer specific prompt imports instead of namespace import.Please import only the used symbol(s) from
~/cli/prompts.js(e.g.,confirm) instead of* as pto align with the project import style.As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/installers/proofkit-auth.ts` at line 9, Replace the namespace import "import * as p from \"~/cli/prompts.js\";" with specific named imports for only the prompts you use (e.g., "import { confirm } from \"~/cli/prompts.js\";"), updating the import to list each used symbol instead of the namespace so code (and any references to p.confirm, p.input, etc.) uses the direct names (adjust call sites like p.confirm -> confirm if necessary).packages/cli/src/installers/proofkit-webviewer.ts (1)
6-6: Prefer explicit prompt imports here as well.Import only
confirm(or other directly used symbols) instead of* as pfor consistency and clarity.As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/installers/proofkit-webviewer.ts` at line 6, Replace the namespace import "import * as p from '~/cli/prompts.js';" with explicit imports for only the prompt functions you use (e.g., "import { confirm } from '~/cli/prompts.js';"), and update any call sites that use p.confirm (or other p.*) to call confirm directly; if multiple prompt symbols are used, list them explicitly in the import to avoid the namespace import and maintain consistency with the project's import style.packages/cli/src/cli/add/data-source/index.ts (1)
3-3: Prefer a directselectimport.Since only
selectis used, replace the namespace import with a named import for consistency.As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/add/data-source/index.ts` at line 3, The file uses a namespace import "import * as p from \"~/cli/prompts.js\"" but only the select symbol is needed; replace the namespace import with a named import for select (i.e., import { select } from "~/cli/prompts.js") and update any usages that reference p.select to call select directly (look for p.select occurrences in this module).packages/cli/src/installers/react-email.ts (1)
6-6: Use named prompt imports for this installer.Only
textis needed here, so importing it directly is the better fit.As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/installers/react-email.ts` at line 6, Replace the namespace import of prompts (import * as p from "~/cli/prompts.js") with a named import for only the used prompt: import { text } from "~/cli/prompts.js"; update any references to p.text(...) to just text(...), e.g., in the installer functions that call p.text ensure they call text directly so the file uses specific imports instead of a namespace import.packages/cli/src/cli/deploy/index.ts (1)
7-7: Use named imports for prompt helpers.Switch
import * as pto explicit imports (spinner,select,isCancel) for consistency with repository conventions.As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/deploy/index.ts` at line 7, Replace the namespace import of the prompt helpers with explicit named imports: change the current `import * as p from "~/cli/prompts.js";` to import the specific helpers used in this module (e.g., `spinner`, `select`, `isCancel`) and update all usages like `p.spinner`, `p.select`, `p.isCancel` to use the direct identifiers `spinner`, `select`, `isCancel` (look for references in this file such as any calls to p.spinner, p.select, or p.isCancel and update them accordingly).packages/new/src/core/resolveInitRequest.ts (2)
68-77: URL constructor used solely for validation.The
new URL(normalized)on line 71 is constructed only to validate the URL format (triggering an exception if invalid), but the result is discarded. A brief comment would clarify this intent for future readers.♻️ Add clarifying comment
validate: (value) => { try { const normalized = value.startsWith("http") ? value : `https://${value}`; + // Validate URL format by attempting construction new URL(normalized); return; } catch {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/core/resolveInitRequest.ts` around lines 68 - 77, The validate arrow function currently constructs new URL(normalized) solely to trigger validation and discards the result; add a concise inline comment above that call in the validate function (the one that computes normalized from value) stating that new URL(...) is intentionally used only for validation and its return value is ignored, so future readers won't think the URL object is needed elsewhere.
364-410: Consider type-safe alternatives to type assertions.The type assertions on lines 382 and 409 cast prompt results to
AppTypeandDataSourceType. Since theselectprompt returns the value from the options array, the types are already constrained, but the assertions bypass TypeScript's type checking.♻️ Type-safe alternative using generics
The
prompt.selectalready accepts a generic type parameter<T extends string>. You could define the options arrays separately with explicit types:if (!(flags.appType || nonInteractive)) { + const appTypeOptions = [ + { + value: "browser" as const, + label: "Web App for Browsers", + hint: "Uses Next.js and hosted deployment", + }, + { + value: "webviewer" as const, + label: "FileMaker Web Viewer", + hint: "Uses Vite for FileMaker web viewers", + }, + ]; appType = yield* Effect.promise(() => - prompt.select({ + prompt.select<AppType>({ message: "What kind of app do you want to build?", - options: [ - { - value: "browser", - label: "Web App for Browsers", - hint: "Uses Next.js and hosted deployment", - }, - { - value: "webviewer", - label: "FileMaker Web Viewer", - hint: "Uses Vite for FileMaker web viewers", - }, - ], + options: appTypeOptions, }), - ).pipe(Effect.map((value) => value as AppType)); + ); }As per coding guidelines: "Use const assertions (
as const) for immutable values and literal types" and "Leverage TypeScript's type narrowing instead of type assertions".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/core/resolveInitRequest.ts` around lines 364 - 410, The prompt.select results are being force-cast to AppType and DataSourceType (appType and dataSource); instead, make the select calls type-safe by providing a generic type parameter and strongly-typed options arrays (e.g., define the options for app type and data source as const arrays with explicit literal types or use prompt.select<AppType>(...) and prompt.select<DataSourceType>(...)); update the two select invocations that set appType and dataSource in resolveInitRequest.ts to use these typed option arrays and the generic form of prompt.select so you can remove the `as AppType` and `as DataSourceType` assertions while keeping compile-time safety.packages/new/tests/cli.test.ts (2)
12-18: Consider usingbeforeAllto build once instead of per-test.The
buildCli()function is called at the start of each test, which rebuilds the entire package three times during the test run. Moving the build to abeforeAllhook would significantly speed up the test suite.♻️ Suggested optimization
+import { beforeAll, describe, expect, it } from "vitest"; -import { describe, expect, it } from "vitest"; -function buildCli() { - execFileSync("pnpm", ["build"], { - cwd: packageDir, - stdio: "pipe", - encoding: "utf8", - }); -} - describe("proofkit-new CLI", () => { + beforeAll(() => { + execFileSync("pnpm", ["build"], { + cwd: packageDir, + stdio: "pipe", + encoding: "utf8", + }); + }); + it("shows kebab-case init flags in help", () => { - buildCli(); const output = execFileSync("node", [distEntry, "init", "--help"], {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/cli.test.ts` around lines 12 - 18, The tests currently call buildCli() inside each test, rebuilding the package multiple times; instead, move the heavy build step to a beforeAll hook so it runs once for the suite: add a beforeAll(async () => { buildCli(); }) (or synchronous equivalent) in packages/new/tests/cli.test.ts and remove any per-test calls to buildCli() from individual it/test blocks; reference the existing buildCli() helper to perform the build and keep per-test setup/teardown minimal so tests reuse the single build.
53-53: Fragile assertion on ASCII art header.The assertion
expect(output).toContain("_______")relies on the specific ASCII art pattern of the CLI header, which could break if the header design changes. Consider asserting on more stable output like a version string or known welcome message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/cli.test.ts` at line 53, The test currently asserts the fragile ASCII-art fragment via expect(output).toContain("_______"); update the test in packages/new/tests/cli.test.ts to assert a stable token instead (for example the CLI version string, a known welcome message, or a help header) by checking output for that stable substring (e.g., the version constant or fixed welcome text used by the CLI) rather than the ASCII art; locate the test that reads the variable output and replace the fragile contain check with an assertion against the chosen stable identifier (version string or welcome message).packages/new/tests/executor.test.ts (1)
11-71: Consider cleaning up temporary directories after tests.The tests create temporary directories using
fs.mkdtempbut don't clean them up afterward. While OS tmp directories are eventually cleaned, addingafterEachorafterAllhooks to remove test directories would prevent accumulation during repeated test runs and make tests more self-contained.♻️ Suggested cleanup pattern
+import { afterEach } from "vitest"; + +const tempDirs: string[] = []; + +afterEach(async () => { + for (const dir of tempDirs) { + await fs.remove(dir); + } + tempDirs.length = 0; +}); + describe("executeInitPlan command paths", () => { it("runs install, git, codegen, and filemaker bootstrap through services", async () => { const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "proofkit-new-exec-")); + tempDirs.push(cwd);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/executor.test.ts` around lines 11 - 71, Add cleanup to remove the temporary directories created by fs.mkdtemp in the test: capture the created cwd (from the variable cwd in the "runs install, git, codegen, and filemaker bootstrap through services" test) and add an afterEach or afterAll hook that calls fs.rm/fs.promises.rm (with recursive: true, force: true) to delete cwd and any scaffolded subdir (e.g., path.join(cwd, "fm-app")) to avoid leaving tmp artifacts; ensure the hook runs even when tests fail by using the test framework's afterEach/afterAll lifecycle and reference the same cwd used in the test so removal targets the correct temp folder.packages/new/src/utils/prompts.ts (2)
120-128: Extract magic string to a named constant.The
"__no_matches__"value is a magic string used to represent a disabled placeholder option. Consider extracting it to a named constant for clarity and to prevent accidental typos.♻️ Suggested refactor
+const NO_MATCHES_PLACEHOLDER = "__no_matches__" as const; + export function searchSelectPrompt<T extends string>(options: { // ... source: (input) => { const filtered = filterSearchOptions(options.options, input); if (filtered.length === 0) { return [ { - value: "__no_matches__" as T, + value: NO_MATCHES_PLACEHOLDER as T, name: options.emptyMessage ?? "No matches found. Keep typing to refine your search.", disabled: options.emptyMessage ?? "No matches found", }, ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/prompts.ts` around lines 120 - 128, Extract the magic string "__no_matches__" into a named constant (e.g., NO_MATCHES_VALUE) and replace its inline usage in the empty-results branch where filtered.length === 0; update the returned placeholder option (the object with value, name, disabled) to use that constant (keeping the "as T" cast) so the special placeholder is referenced by a single well-named symbol and avoids accidental typos.
140-156: Function name may be misleading - no search capability.
multiSearchSelectPromptusesinquirerCheckboxwhich doesn't provide search/filter functionality. The name suggests search capability that isn't present. Consider renaming tomultiSelectPromptor implementing actual search filtering.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/prompts.ts` around lines 140 - 156, The function multiSearchSelectPrompt is misnamed because it uses inquirerCheckbox (no search/filter), so either rename the function to multiSelectPrompt (and update all imports/usages and any exported names) or implement actual search/filter behavior by replacing inquirerCheckbox with a searchable prompt component and wiring the options through a search-enabled API; locate the function multiSearchSelectPrompt and change its exported identifier and caller references or swap in a search-capable prompt implementation so the name matches functionality.packages/new/src/core/executeInitPlan.ts (2)
64-68: Consider using Effect.fail for error propagation in Effect context.Inside an
Effect.genfunction, throwing a plainErrorworks but bypasses the Effect error channel. Usingyield* Effect.fail(...)would be more idiomatic and allows typed error handling.♻️ Suggested refactor
if (cliContext.nonInteractive) { - throw new Error( + return yield* Effect.fail(new Error( `${plan.request.appDir} already exists and isn't empty. Remove the existing files or choose a different directory.`, - ); + )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/core/executeInitPlan.ts` around lines 64 - 68, Replace the plain throw inside the Effect.gen flow with an Effect.fail to propagate errors through the Effect error channel: locate the non-interactive check in executeInitPlan.ts (the cliContext.nonInteractive conditional referencing plan.request.appDir) and change the thrown Error to a yield* Effect.fail(...) (or equivalent Effect.failSync) carrying the same error message or a typed error value so the error flows through the Effect runtime instead of escaping via throw.
129-133: Type assertions withas neverindicate a type mismatch.The double
as nevercast suggests the types betweenreadJsonandapplyPackageJsonMutationsdon't align. Consider using a proper type guard or adjusting the generic type parameter ofreadJsonto matchPackageJson.♻️ Suggested improvement
- const packageJson = yield* Effect.promise(() => fs.readJson<Record<string, unknown>>(packageJsonPath)); - const updatedPackageJson = sortPackageJson( - applyPackageJsonMutations(packageJson as never, plan.packageJson) as never, - ); + const packageJson = yield* Effect.promise(() => fs.readJson<import("type-fest").PackageJson>(packageJsonPath)); + const updatedPackageJson = sortPackageJson(applyPackageJsonMutations(packageJson, plan.packageJson));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/core/executeInitPlan.ts` around lines 129 - 133, The double "as never" casts show a type mismatch between the JSON read and the package JSON shape; update the read/write generics and remove the casts by reading with a concrete PackageJson type (e.g., fs.readJson<PackageJson>(packageJsonPath)) and/or validate the parsed object with a type guard or schema (e.g., runtime validator) before passing it to applyPackageJsonMutations; ensure applyPackageJsonMutations and sortPackageJson accept PackageJson and write back the validated PackageJson to fs.writeJson(packageJsonPath, updatedPackageJson) so no `as never` casts are needed (referencing packageJson, applyPackageJsonMutations, sortPackageJson, fs.readJson, fs.writeJson, packageJsonPath, plan.packageJson).packages/new/src/core/planInit.ts (1)
86-103: Extract duplicated condition into a named boolean.The complex condition for determining whether to run codegen appears twice (lines 88-91 and 98-101). Extract it into a well-named variable for clarity and DRY compliance.
♻️ Suggested refactor
+ const shouldRunCodegen = + request.dataSource === "filemaker" && + !request.skipFileMakerSetup && + !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs); + return { // ... commands: [ ...(request.noInstall ? [] : [{ type: "install" as const }]), - ...(request.dataSource === "filemaker" && - !request.skipFileMakerSetup && - !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs) - ? [{ type: "codegen" as const }] - : []), + ...(shouldRunCodegen ? [{ type: "codegen" as const }] : []), ...(request.noGit ? [] : [{ type: "git-init" as const }]), ], tasks: { bootstrapFileMaker: request.dataSource === "filemaker" && !request.skipFileMakerSetup, runInstall: !request.noInstall, - runInitialCodegen: - request.dataSource === "filemaker" && - !request.skipFileMakerSetup && - !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs), + runInitialCodegen: shouldRunCodegen, initializeGit: !request.noGit, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/core/planInit.ts` around lines 86 - 103, The duplicated complex condition for running FileMaker codegen should be extracted into a well-named boolean (e.g., runFileMakerCodegen or shouldRunFileMakerCodegen) defined above the commands/tasks block; replace both occurrences in commands (the conditional that yields { type: "codegen" as const }) and in tasks (runInitialCodegen and bootstrapFileMaker if appropriate) with that boolean so the logic is centralized and the "codegen" literal and other flags (request.skipFileMakerSetup, request.appType, request.nonInteractive, request.hasExplicitFileMakerInputs) remain unchanged.packages/new/src/utils/projectFiles.ts (2)
78-98: Directory detection heuristic may miss some edge cases.The logic
!(extension || entry.includes("."))to detect directories is fragile. Files without extensions that don't contain.would be treated as directories, and dotfiles (other than.gitignore) are skipped. Consider usingfs.statorfs.lstatfor reliable directory detection.♻️ More robust approach
export async function replaceTextInFiles( fs: { readdir: (path: string) => Promise<string[]>; readFile: (path: string) => Promise<string>; writeFile: (path: string, content: string) => Promise<void>; + stat?: (path: string) => Promise<{ isDirectory: () => boolean }>; }, rootDir: string, searchValue: string, replaceValue: string, ) { const entries = await fs.readdir(rootDir); for (const entry of entries) { const fullPath = path.join(rootDir, entry); const extension = path.extname(entry); - if (!(extension || entry.includes("."))) { + // If fs.stat is available, use it for reliable detection + if (fs.stat) { + const stats = await fs.stat(fullPath).catch(() => null); + if (stats?.isDirectory()) { + await replaceTextInFiles(fs, fullPath, searchValue, replaceValue).catch(() => undefined); + continue; + } + } else if (!(extension || entry.includes("."))) { await replaceTextInFiles(fs, fullPath, searchValue, replaceValue).catch(() => undefined); continue; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/projectFiles.ts` around lines 78 - 98, The directory-detection using !(extension || entry.includes(".")) is unreliable; instead call fs.lstat (or fs.stat) on fullPath to check isDirectory() and branch: if it's a directory, await replaceTextInFiles(fs, fullPath, searchValue, replaceValue); else treat it as a file (including dotfiles with no extension) and continue to check textFileExtensions, then read with fs.readFile and write with fs.writeFile as currently done; update the loop around entries, fullPath, textFileExtensions, replaceTextInFiles, and fs.readFile/writeFile to use the lstat-based check.
100-137: String-based code modification is fragile but acceptable for controlled templates.The marker-based insertion (
" server: {"and" },") works for scaffolded files but would break if the template format changes. Consider adding a comment in the template file to make the insertion point explicit (e.g.,// PROOFKIT_ENV_ENTRIES).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/projectFiles.ts` around lines 100 - 137, The current updateEnvSchemaFile function uses fragile string markers (" server: {" and " },") to find the insertion point; change it to look for a dedicated comment marker (e.g., "// PROOFKIT_ENV_ENTRIES") in the env template and update the code in updateEnvSchemaFile to search for that exact comment (use a clear unique token), fall back gracefully if not found, and insert the generated additions at that comment position instead of the "server" block; update the template file to include the comment marker so future changes won't break the insertion.packages/new/tests/test-layer.ts (1)
146-156: Hardcoded template path may be fragile.The path
../../cli/template/is relative to the test file location. Consider using a more robust path resolution or making it configurable via options.♻️ Alternative approach
export function makeTestLayer(options: { cwd: string; packageManager: PackageManager; nonInteractive?: boolean; prompts?: PromptScript; console?: ConsoleTranscript; tracker?: { /* ... */ }; fileMaker?: { /* ... */ }; + templateRoot?: string; }) { + const templateRoot = options.templateRoot ?? path.resolve(__dirname, "../../cli/template"); // ... Layer.succeed(TemplateService, { getTemplateDir: (appType: AppType, ui: UIType) => { // ... - return path.resolve(__dirname, `../../cli/template/${templateName}`); + return path.join(templateRoot, templateName); }, }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/test-layer.ts` around lines 146 - 156, The test currently hardcodes the template directory inside the Layer.succeed(TemplateService, { getTemplateDir(...) }) implementation using path.resolve(__dirname, `../../cli/template/${templateName}`), which is brittle; change getTemplateDir to derive the base template path from a configurable source (e.g., an environment variable or a test option) with a safe fallback to a resolved absolute path, e.g., read process.env.TEST_TEMPLATE_DIR (or a test-provided option) and join it with templateName, and update Layer.succeed(TemplateService...) to use that configurable base so tests don’t rely on fragile relative paths.packages/new/tests/integration.test.ts (1)
12-77: Consider cleaning up temporary directories after tests.The tests create temporary directories using
fs.mkdtempbut don't clean them up afterward. Consider usingafterEachorafterAllhooks to remove test artifacts, or using a test utility that handles cleanup.♻️ Example cleanup pattern
describe("integration scaffold generation", () => { const tempDirs: string[] = []; afterAll(async () => { await Promise.all(tempDirs.map((dir) => fs.remove(dir))); }); it("creates a browser scaffold...", async () => { const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "proofkit-new-browser-")); tempDirs.push(cwd); // ... rest of test }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/integration.test.ts` around lines 12 - 77, The test creates temporary dirs via fs.mkdtemp (cwd) but never removes them; add a tempDirs array at the top of the describe, push cwd into tempDirs immediately after creating it in the test, and add an afterAll (or afterEach) hook that awaits Promise.all(tempDirs.map(dir => fs.remove(dir))) to clean up; reference the existing cwd variable and fs.mkdtemp/fs.remove to locate where to push and remove the temp dirs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b96d5e48-2b4d-4ca7-92dc-31075a032f2d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
packages/cli/package.jsonpackages/cli/src/cli/add/auth.tspackages/cli/src/cli/add/data-source/deploy-demo-file.tspackages/cli/src/cli/add/data-source/filemaker.tspackages/cli/src/cli/add/data-source/index.tspackages/cli/src/cli/add/fmschema.tspackages/cli/src/cli/add/index.tspackages/cli/src/cli/add/page/index.tspackages/cli/src/cli/add/registry/install.tspackages/cli/src/cli/deploy/index.tspackages/cli/src/cli/init.tspackages/cli/src/cli/menu.tspackages/cli/src/cli/ottofms.tspackages/cli/src/cli/prompts.tspackages/cli/src/cli/react-email.tspackages/cli/src/cli/remove/data-source.tspackages/cli/src/cli/remove/index.tspackages/cli/src/cli/remove/page.tspackages/cli/src/cli/remove/schema.tspackages/cli/src/cli/tanstack-query.tspackages/cli/src/cli/utils.tspackages/cli/src/helpers/git.tspackages/cli/src/helpers/scaffoldProject.tspackages/cli/src/index.tspackages/cli/src/installers/proofkit-auth.tspackages/cli/src/installers/proofkit-webviewer.tspackages/cli/src/installers/react-email.tspackages/cli/src/utils/renderVersionWarning.tspackages/new/package.jsonpackages/new/src/consts.tspackages/new/src/core/context.tspackages/new/src/core/errors.tspackages/new/src/core/executeInitPlan.tspackages/new/src/core/planInit.tspackages/new/src/core/resolveInitRequest.tspackages/new/src/core/types.tspackages/new/src/index.tspackages/new/src/services/live.tspackages/new/src/utils/browserOpen.tspackages/new/src/utils/http.tspackages/new/src/utils/packageManager.tspackages/new/src/utils/projectFiles.tspackages/new/src/utils/projectName.tspackages/new/src/utils/prompts.tspackages/new/src/utils/renderTitle.tspackages/new/src/utils/versioning.tspackages/new/tests/cli.test.tspackages/new/tests/default-command.test.tspackages/new/tests/executor.test.tspackages/new/tests/init-fixtures.tspackages/new/tests/integration.test.tspackages/new/tests/planner.test.tspackages/new/tests/prompts.test.tspackages/new/tests/resolve-init.test.tspackages/new/tests/test-layer.tspackages/new/tsconfig.jsonpackages/new/tsdown.config.tspackages/new/vitest.config.tsturbo.json
eluce2
force-pushed
the
codex/proofkit-new-scaffold
branch
from
44dbbf5 to
4040eff
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (4)
packages/new/src/utils/http.ts (2)
51-60:⚠️ Potential issue | 🟠 Major
requestJsonstill treats non-2xx payloads as successful data.With
validateStatus: null(Line 58) andreturn response.data(Line 60), 4xx/5xx responses can flow as if successful results.🔧 Proposed fix
const response = await axios.request<T>({ url: url.toString(), method: options?.method ?? "GET", data: options?.body, headers: options?.headers, httpsAgent, timeout: options?.timeoutMs ?? 10_000, - validateStatus: null, + validateStatus: () => true, }); + if (response.status < 200 || response.status >= 300) { + throw new Error(`HTTP ${response.status} for ${options?.method ?? "GET"} ${url.toString()}`); + } return response.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/http.ts` around lines 51 - 60, The requestJson helper currently calls axios.request<T> with validateStatus: null and returns response.data, allowing 4xx/5xx responses to be treated as success; update the logic in requestJson/axios.request<T> so that non-2xx responses are rejected or cause an error: either set validateStatus to a function that only accepts status >=200 && status <300, or after the request inspect response.status and throw a descriptive error (including response.status and response.data) before returning response.data; ensure the thrown error preserves enough context for callers to handle HTTP errors.
4-6:⚠️ Potential issue | 🔴 CriticalDo not disable TLS verification globally.
Line 5 sets
rejectUnauthorized: false, which allows MITM on all HTTPS requests routed through this helper.🔒 Proposed fix
+const allowInsecureTls = process.env.PROOFKIT_ALLOW_INSECURE_TLS === "1"; const httpsAgent = new https.Agent({ - rejectUnauthorized: false, + rejectUnauthorized: !allowInsecureTls, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/http.ts` around lines 4 - 6, The httpsAgent constant currently disables TLS verification (rejectUnauthorized: false) which is unsafe; change the httpsAgent definition to enable verification (rejectUnauthorized: true) or remove the custom agent so Node's default verification is used, and if you need to allow self-signed certs for local testing make it opt-in only (e.g., controlled by an explicit env var like ALLOW_INSECURE_TLS or by checking NODE_ENV !== 'production') so the insecure setting is never active in production; update references to httpsAgent accordingly.packages/cli/src/cli/prompts.ts (1)
82-95:⚠️ Potential issue | 🟡 MinorThese wrapper parameters are still no-ops.
text.placeholder,confirm.active/inactive, andsearchSelect.searchLabelare still accepted but never reach the inquirer call, so the new call sites in this PR lose those prompt hints at runtime.Also applies to: 106-114, 137-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/prompts.ts` around lines 82 - 95, The wrapper functions (e.g., text) accept parameters like placeholder (and similarly confirm.active/inactive and searchSelect.searchLabel) but never forward them to the underlying inquirer calls, so those UI hints are lost; update each wrapper (text -> inquirerInput, confirm wrapper -> its inquirer confirm call, searchSelect wrapper -> its inquirer search/select call) to pass through the corresponding options properties (placeholder, active/inactive, searchLabel) into the inquirer call arguments and keep existing normalize/withCancelSentinel behavior so the values are preserved at runtime.packages/new/src/services/live.ts (1)
507-522:⚠️ Potential issue | 🟠 MajorStill missing a timeout on deployment polling.
If the deployment never reaches a terminal state, init hangs forever and the spinner never stops. Bound this loop with a deadline or max-attempt counter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/services/live.ts` around lines 507 - 522, The infinite polling loop around fileMakerService.getDeploymentStatus needs a bounded timeout or max-attempts guard: change the while (true) loop in this block to track attempts or compute a deadline (e.g., const maxAttempts = N or const deadline = Date.now() + TIMEOUT_MS) and break/throw when exceeded; when timing out call spin.stop("Demo deployment timed out") and throw a clear Error (e.g., "ProofKit Demo deployment timed out"), otherwise continue existing logic that checks status.data.response.running and complete; reference the polling code that calls fileMakerService.getDeploymentStatus and uses spin.stop to locate where to add the counter/deadline and timeout handling.
🧹 Nitpick comments (5)
packages/new/src/utils/renderTitle.ts (1)
5-9: Use a const assertion for immutable theme literals.The theme object is effectively immutable;
as constmakes that explicit and tightens typing.♻️ Proposed refactor
const proofTheme = { purple: "#89216B", lightPurple: "#D15ABB", orange: "#FF595E", -}; +} as const;As per coding guidelines, "Use const assertions (
as const) for immutable values and literal types".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/renderTitle.ts` around lines 5 - 9, The proofTheme object should be declared with a const assertion to indicate immutability and tighten its literal types; update the declaration of proofTheme (the object with keys purple, lightPurple, orange) to use a const assertion (i.e., add "as const" to the object expression) so the values become readonly literal types and the compiler infers exact string literals rather than widened string types.packages/cli/src/helpers/scaffoldProject.ts (1)
5-5: Use named prompt imports instead of a namespace import.Line 5 uses
import * as p, which makes callsites less explicit (p.select,p.confirm). Prefer named imports for clarity and better static analysis.♻️ Proposed refactor
-import * as p from "~/cli/prompts.js"; +import { confirm, select } from "~/cli/prompts.js"; ... - const overwriteDir = await p.select({ + const overwriteDir = await select({ ... - const confirmOverwriteDir = await p.confirm({ + const confirmOverwriteDir = await confirm({As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/helpers/scaffoldProject.ts` at line 5, Replace the namespace import "import * as p from '~/cli/prompts.js';" with named imports for the specific prompt helpers used in this module (e.g., select, confirm, input, etc.) so callsites become explicit (select(...), confirm(...)). Update the import to list only the functions actually referenced in scaffoldProject.ts and adjust any usages accordingly to remove the "p." namespace.packages/cli/src/cli/add/registry/install.ts (1)
5-5: Prefer named imports for prompt helpers.Line 5 uses a namespace import for prompts. Named imports make usage clearer and align with project conventions.
♻️ Proposed refactor
-import * as p from "~/cli/prompts.js"; +import { select, text, cancel } from "~/cli/prompts.js";As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/add/registry/install.ts` at line 5, Replace the namespace import "import * as p from '~/cli/prompts.js'" with direct named imports of only the prompt helpers this module uses (e.g., prompt, confirm, select) so usages become explicit; update all references like p.prompt/p.confirm to the corresponding named identifiers and remove the namespace import to comply with the "prefer specific imports" guideline.packages/new/tests/cli.test.ts (1)
12-23: Build the CLI once per suite, not once per test.
buildCli()is called in every test, which adds avoidable process overhead. Move it tobeforeAllto keep test runtime down.♻️ Proposed refactor
-import { describe, expect, it } from "vitest"; +import { beforeAll, describe, expect, it } from "vitest"; ... describe("proofkit-new CLI", () => { + beforeAll(() => { + buildCli(); + }); + it("shows kebab-case init flags in help", () => { - buildCli(); const output = execFileSync("node", [distEntry, "init", "--help"], { ... it("prints the header and coming-soon message when run inside a ProofKit project", async () => { - buildCli(); const cwd = await fs.mkdtemp(path.join(os.tmpdir(), "proofkit-new-cli-project-")); ... it("fails with guidance when no command is used in non-interactive mode", () => { - buildCli(); const result = spawnSync("node", [distEntry, "--non-interactive"], {Also applies to: 37-38, 59-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/cli.test.ts` around lines 12 - 23, Tests currently call buildCli() inside each it block causing repeated builds; move the build out to run once per suite by adding a beforeAll hook inside the describe("proofkit-new CLI") block that calls buildCli(), and remove the per-test buildCli() calls from the individual it tests (including the other tests that currently call buildCli() such as the ones showing kebab-case init flags and the additional tests that call it). Keep the existing synchronous behavior of buildCli() so tests rely on the completed build before executing.packages/cli/src/cli/tanstack-query.ts (1)
2-2: Prefer a named prompt import here.Only
spinner()is used in this file, so the namespace import adds indirection without buying much.♻️ Minimal refactor
-import * as p from "~/cli/prompts.js"; +import { spinner as createSpinner } from "~/cli/prompts.js";- const spinner = p.spinner(); + const spinner = createSpinner();As per coding guidelines, "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/tanstack-query.ts` at line 2, The file currently imports the prompts module as a namespace (import * as p from "~/cli/prompts.js") but only uses spinner(), so change to a named import (import { spinner } from "~/cli/prompts.js") and update any usage sites that call p.spinner() to call spinner() directly; ensure there are no other references to p.* left and run the file's tests or typecheck to confirm no unresolved symbols remain.
🤖 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/cli/add/registry/install.ts`:
- Line 49: The current mapping for options uses schemas.map((schema) => ({
label: schema ?? "", value: schema ?? "" })) which can emit empty-string entries
when schema is nullish; change this by narrowing/filtering the schemas array to
only actual strings before mapping (e.g., use a type guard like filter((s): s is
string => typeof s === "string") or equivalent) and then map to { label: schema,
value: schema } so options contains only valid string entries; update the code
that builds the options property (the options assignment using schemas) to
perform the filter-then-map flow.
In `@packages/new/src/core/resolveInitRequest.ts`:
- Around line 385-390: The dataSource is being set before evaluating
hasExplicitFileMakerInputs, causing explicit FileMaker flags to be ignored; move
or compute hasExplicitFileMakerInputs prior to the dataSource assignment and use
it in the dataSource selection logic (for the block that sets dataSource when
flags.dataSource is falsy and appType === "webviewer") so that nonInteractive
runs which include explicit FileMaker inputs (flags.server, flags.fileName,
flags.dataApiKey) correctly choose "filemaker" instead of "none"; apply the same
ordering/logic correction to the other similar dataSource assignment block that
references the same condition (the second occurrence around where dataSource is
set again).
- Around line 82-99: The code currently forces Otto discovery
(compareSemver/versions.ottoVersion, fileMakerService.getOttoFMSToken and
fileMakerService.listFiles) before honoring explicit hosted inputs; reorder
logic so you first check flags.fileName and flags.dataApiKey and the
nonInteractive branch (using selectedFile and nonInteractive) and only perform
Otto auto-login/enumeration when hosted inputs are missing or discovery is
explicitly required. Concretely, keep variables demoFileName and selectedFile,
move the block that checks !(flags.adminApiKey || (versions.ottoVersion &&
compareSemver(...))) and the calls to fileMakerService.getOttoFMSToken/listFiles
into an if that runs only when selectedFile or data API key are not provided,
and ensure the "No hosted FileMaker files" and related errors are only raised
inside that discovery branch.
In `@packages/new/src/index.ts`:
- Around line 80-84: Inside the Effect.gen block where you check
cliContext.nonInteractive || flags.CI || flags.nonInteractive, replace the
direct throw new Error(...) with yielding an Effect failure so the error goes
through the Effect error channel; e.g. use yield* Effect.fail(new Error("The
default command is interactive-only in non-interactive mode. Run an explicit
command such as `proofkit-new init <name> --non-interactive`.")) (and ensure
Effect is imported/accessible). Target the conditional around
cliContext/nonInteractive and flags and use Effect.fail instead of throw to
preserve proper typed error handling and composition.
In `@packages/new/src/services/live.ts`:
- Around line 67-72: The flatten function currently uses the `"isFolder" in
layout` check which treats objects with isFolder: false as folders and returns
no names; change the condition to check the boolean like `layout.isFolder ===
true` (or simply `if (layout.isFolder)`) so only real folders are recursed into,
and keep the leaf branch returning `layout.name` when `isFolder` is false;
update the recursion to safely handle `layout.folderLayoutNames` (Array.isArray)
and call flatten for each item as before to ensure leaf layout names are
preserved (refer to the flatten function and the LayoutFolder type).
- Around line 317-329: The first probe using getJson to call "/otto/api/info"
can throw and currently aborts before the Otto 3 fallback; wrap the Otto 4 probe
(the call that assigns otto4Response and ottoVersion) in a try/catch that
swallows or logs the error and leaves ottoVersion null so execution continues to
the Otto 3 probe (referencing otto4Response, getJson, ottoVersion); only after
attempting both probes (the Otto 4 try/catch and the subsequent
otto3Url/otto3Response getJson call) should you treat the discovery as failed.
In `@packages/new/src/utils/browserOpen.ts`:
- Line 3: The exported function openBrowser currently lacks an explicit return
type—add ": Promise<void>" to the openBrowser signature to make the async API
contract explicit, and update the exported alias openExternal to have an
explicit function type (e.g., a typed const or exported function signature
matching openBrowser: Promise<void>) so both exported symbols are explicitly
typed; locate and change the openBrowser declaration and the openExternal
export/alias to include these types.
In `@packages/new/src/utils/packageManager.ts`:
- Line 19: The package-manager fallback in packageManager.ts is inconsistent:
the earlier branch treats unknown agents as "npm" but the missing-UA branch
returns "pnpm" (the line returning "pnpm"); update the function that detects the
package manager so that when process.env.npm_config_user_agent is absent or
unrecognizable it returns the same default ("npm") as the unknown-agent branch
(replace the return "pnpm" with return "npm" and ensure any helper like
getPackageManager/detectPackageManager uses this single consistent fallback).
In `@packages/new/src/utils/prompts.ts`:
- Around line 109-137: searchSelectPrompt declares a searchLabel option but
never passes it to inquirerSearch, so callers' label is ignored; update the
inquirerSearch call in searchSelectPrompt to include the passed-in
options.searchLabel (falling back to an appropriate default if undefined) as the
searchLabel property so the prompt actually displays the provided search label;
reference the searchSelectPrompt function and the options.searchLabel parameter
when making this change.
- Around line 18-23: The module currently re-exports isCancel from clack but
never produces a cancel sentinel it recognizes; define a local CANCEL_SYMBOL and
implement isPromptCancel() and withCancelSentinel() that catches ExitPromptError
and returns CANCEL_SYMBOL, then wrap all inquirer-backed helpers (textPrompt,
passwordPrompt, confirmPrompt, selectPrompt, searchSelectPrompt,
multiSearchSelectPrompt) with withCancelSentinel() so they return the unified
cancel sentinel and callers can use isPromptCancel/isCancel consistently; also
remove the unused searchLabel option from searchSelectPrompt's options list and
update any references to use the new isPromptCancel (or export an alias) so the
module exposes a single abort contract.
In `@packages/new/tests/planner.test.ts`:
- Line 12: The test asserts a POSIX path literal for plan.targetDir which breaks
on Windows; update the expectation to compute the expected path using Node's
path utilities (e.g., call path.resolve(...) or path.join(...) with the same
segments used by planInit) and import/require path in the test file so the
assertion compares path.resolve(...expectedSegments) to plan.targetDir instead
of the hard-coded "/tmp/workspace/demo-app".
---
Duplicate comments:
In `@packages/cli/src/cli/prompts.ts`:
- Around line 82-95: The wrapper functions (e.g., text) accept parameters like
placeholder (and similarly confirm.active/inactive and searchSelect.searchLabel)
but never forward them to the underlying inquirer calls, so those UI hints are
lost; update each wrapper (text -> inquirerInput, confirm wrapper -> its
inquirer confirm call, searchSelect wrapper -> its inquirer search/select call)
to pass through the corresponding options properties (placeholder,
active/inactive, searchLabel) into the inquirer call arguments and keep existing
normalize/withCancelSentinel behavior so the values are preserved at runtime.
In `@packages/new/src/services/live.ts`:
- Around line 507-522: The infinite polling loop around
fileMakerService.getDeploymentStatus needs a bounded timeout or max-attempts
guard: change the while (true) loop in this block to track attempts or compute a
deadline (e.g., const maxAttempts = N or const deadline = Date.now() +
TIMEOUT_MS) and break/throw when exceeded; when timing out call spin.stop("Demo
deployment timed out") and throw a clear Error (e.g., "ProofKit Demo deployment
timed out"), otherwise continue existing logic that checks
status.data.response.running and complete; reference the polling code that calls
fileMakerService.getDeploymentStatus and uses spin.stop to locate where to add
the counter/deadline and timeout handling.
In `@packages/new/src/utils/http.ts`:
- Around line 51-60: The requestJson helper currently calls axios.request<T>
with validateStatus: null and returns response.data, allowing 4xx/5xx responses
to be treated as success; update the logic in requestJson/axios.request<T> so
that non-2xx responses are rejected or cause an error: either set validateStatus
to a function that only accepts status >=200 && status <300, or after the
request inspect response.status and throw a descriptive error (including
response.status and response.data) before returning response.data; ensure the
thrown error preserves enough context for callers to handle HTTP errors.
- Around line 4-6: The httpsAgent constant currently disables TLS verification
(rejectUnauthorized: false) which is unsafe; change the httpsAgent definition to
enable verification (rejectUnauthorized: true) or remove the custom agent so
Node's default verification is used, and if you need to allow self-signed certs
for local testing make it opt-in only (e.g., controlled by an explicit env var
like ALLOW_INSECURE_TLS or by checking NODE_ENV !== 'production') so the
insecure setting is never active in production; update references to httpsAgent
accordingly.
---
Nitpick comments:
In `@packages/cli/src/cli/add/registry/install.ts`:
- Line 5: Replace the namespace import "import * as p from '~/cli/prompts.js'"
with direct named imports of only the prompt helpers this module uses (e.g.,
prompt, confirm, select) so usages become explicit; update all references like
p.prompt/p.confirm to the corresponding named identifiers and remove the
namespace import to comply with the "prefer specific imports" guideline.
In `@packages/cli/src/cli/tanstack-query.ts`:
- Line 2: The file currently imports the prompts module as a namespace (import *
as p from "~/cli/prompts.js") but only uses spinner(), so change to a named
import (import { spinner } from "~/cli/prompts.js") and update any usage sites
that call p.spinner() to call spinner() directly; ensure there are no other
references to p.* left and run the file's tests or typecheck to confirm no
unresolved symbols remain.
In `@packages/cli/src/helpers/scaffoldProject.ts`:
- Line 5: Replace the namespace import "import * as p from '~/cli/prompts.js';"
with named imports for the specific prompt helpers used in this module (e.g.,
select, confirm, input, etc.) so callsites become explicit (select(...),
confirm(...)). Update the import to list only the functions actually referenced
in scaffoldProject.ts and adjust any usages accordingly to remove the "p."
namespace.
In `@packages/new/src/utils/renderTitle.ts`:
- Around line 5-9: The proofTheme object should be declared with a const
assertion to indicate immutability and tighten its literal types; update the
declaration of proofTheme (the object with keys purple, lightPurple, orange) to
use a const assertion (i.e., add "as const" to the object expression) so the
values become readonly literal types and the compiler infers exact string
literals rather than widened string types.
In `@packages/new/tests/cli.test.ts`:
- Around line 12-23: Tests currently call buildCli() inside each it block
causing repeated builds; move the build out to run once per suite by adding a
beforeAll hook inside the describe("proofkit-new CLI") block that calls
buildCli(), and remove the per-test buildCli() calls from the individual it
tests (including the other tests that currently call buildCli() such as the ones
showing kebab-case init flags and the additional tests that call it). Keep the
existing synchronous behavior of buildCli() so tests rely on the completed build
before executing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7b10b0fc-f266-4522-b174-874d989c2352
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (59)
packages/cli/package.jsonpackages/cli/src/cli/add/auth.tspackages/cli/src/cli/add/data-source/deploy-demo-file.tspackages/cli/src/cli/add/data-source/filemaker.tspackages/cli/src/cli/add/data-source/index.tspackages/cli/src/cli/add/fmschema.tspackages/cli/src/cli/add/index.tspackages/cli/src/cli/add/page/index.tspackages/cli/src/cli/add/registry/install.tspackages/cli/src/cli/deploy/index.tspackages/cli/src/cli/init.tspackages/cli/src/cli/menu.tspackages/cli/src/cli/ottofms.tspackages/cli/src/cli/prompts.tspackages/cli/src/cli/react-email.tspackages/cli/src/cli/remove/data-source.tspackages/cli/src/cli/remove/index.tspackages/cli/src/cli/remove/page.tspackages/cli/src/cli/remove/schema.tspackages/cli/src/cli/tanstack-query.tspackages/cli/src/cli/utils.tspackages/cli/src/helpers/git.tspackages/cli/src/helpers/scaffoldProject.tspackages/cli/src/index.tspackages/cli/src/installers/proofkit-auth.tspackages/cli/src/installers/proofkit-webviewer.tspackages/cli/src/installers/react-email.tspackages/cli/src/utils/renderVersionWarning.tspackages/new/package.jsonpackages/new/src/consts.tspackages/new/src/core/context.tspackages/new/src/core/errors.tspackages/new/src/core/executeInitPlan.tspackages/new/src/core/planInit.tspackages/new/src/core/resolveInitRequest.tspackages/new/src/core/types.tspackages/new/src/index.tspackages/new/src/services/live.tspackages/new/src/utils/browserOpen.tspackages/new/src/utils/http.tspackages/new/src/utils/packageManager.tspackages/new/src/utils/projectFiles.tspackages/new/src/utils/projectName.tspackages/new/src/utils/prompts.tspackages/new/src/utils/renderTitle.tspackages/new/src/utils/versioning.tspackages/new/tests/cli.test.tspackages/new/tests/default-command.test.tspackages/new/tests/executor.test.tspackages/new/tests/init-fixtures.tspackages/new/tests/integration.test.tspackages/new/tests/planner.test.tspackages/new/tests/prompts.test.tspackages/new/tests/resolve-init.test.tspackages/new/tests/test-layer.tspackages/new/tsconfig.jsonpackages/new/tsdown.config.tspackages/new/vitest.config.tsturbo.json
✅ Files skipped from review due to trivial changes (2)
- packages/cli/src/cli/react-email.ts
- packages/cli/src/cli/ottofms.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- packages/new/src/utils/versioning.ts
- packages/cli/src/cli/deploy/index.ts
- packages/cli/src/utils/renderVersionWarning.ts
- packages/new/tests/default-command.test.ts
- packages/new/tests/executor.test.ts
- packages/cli/src/cli/remove/page.ts
- packages/new/vitest.config.ts
- packages/cli/package.json
- packages/cli/src/cli/add/page/index.ts
- packages/new/tsconfig.json
- turbo.json
- packages/cli/src/installers/react-email.ts
- packages/cli/src/installers/proofkit-webviewer.ts
- packages/new/package.json
- packages/new/src/utils/projectName.ts
- packages/new/src/consts.ts
- packages/cli/src/installers/proofkit-auth.ts
- packages/new/tests/resolve-init.test.ts
- packages/new/src/core/executeInitPlan.ts
- packages/cli/src/cli/utils.ts
- packages/cli/src/cli/remove/data-source.ts
- packages/cli/src/cli/add/index.ts
- packages/new/src/utils/projectFiles.ts
- packages/cli/src/cli/menu.ts
- packages/cli/src/cli/remove/index.ts
- packages/cli/src/helpers/git.ts
- packages/cli/src/cli/add/data-source/index.ts
- packages/cli/src/cli/remove/schema.ts
- packages/cli/src/cli/add/data-source/deploy-demo-file.ts
- packages/new/src/core/planInit.ts
- packages/new/src/core/errors.ts
@proofkit/better-auth
@proofkit/cli
create-proofkit
@proofkit/fmdapi
@proofkit/fmodata
@proofkit/typegen
@proofkit/webviewer
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
packages/new/tests/project-name.test.ts (1)
4-23: Consider adding tests for scoped package names.The current tests cover Windows paths and
"."handling well. For completeness, consider adding tests for scoped packages:it("parses scoped package names", () => { expect(parseNameAndPath("apps/@myorg/my-app")).toEqual(["@myorg/my-app", "apps/my-app"]); }); it("validates scoped package names", () => { expect(validateAppName("@myorg/my-app")).toBeUndefined(); expect(validateAppName("@INVALID/my-app")).toBe("Name must consist of only lowercase alphanumeric characters, '-', and '_'"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/tests/project-name.test.ts` around lines 4 - 23, Add unit tests covering scoped package names: add a test for parseNameAndPath ensuring input like "apps/@myorg/my-app" returns ["@myorg/my-app","apps/my-app"], and add tests for validateAppName that assert a valid scoped name (e.g., "@myorg/my-app") returns undefined while an invalid scope (e.g., "@INVALID/my-app") returns the same validation error string; use parseNameAndPath and validateAppName in the new cases and keep the test structure consistent with the existing describe block and vi.restoreAllMocks setup.packages/new/src/utils/projectName.ts (1)
40-55: Error message is slightly narrower than what the regex accepts.The error message states "lowercase alphanumeric characters, '-', and '_'" but
VALID_APP_NAME_REGEXalso allows.,~, and*in valid positions. Consider updating the message to accurately reflect all allowed characters, or simplify to something like "App name is not valid."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/projectName.ts` around lines 40 - 55, The error message returned when VALID_APP_NAME_REGEX.test(scopedAppName ?? "") fails is too narrow; update the returned string (the literal error message in the function that uses VALID_APP_NAME_REGEX and scopedAppName/normalized/segments logic) so it accurately reflects all allowed characters (include ".", "~", and "*" in the description) or replace it with a simpler, accurate message such as "App name is not valid."; ensure you update the exact string returned in the branch after VALID_APP_NAME_REGEX.test to match the regex semantics.packages/cli/src/cli/add/fmschema.ts (1)
8-8: Use specific imports from the prompts module.The namespace import makes callsites less explicit. Refactor to import only the used exports:
select,isCancel,cancel,spinner,note,searchSelect,text, andoutro.Per coding guidelines: "Prefer specific imports over namespace imports".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/cli/add/fmschema.ts` at line 8, Replace the namespace import "import * as p from \"~/cli/prompts.js\";" with a named import that explicitly imports the used prompt utilities: select, isCancel, cancel, spinner, note, searchSelect, text, and outro; then update any callsites that reference p.<name> to use the direct identifiers (e.g., select(...), isCancel(...), cancel(...), spinner(...), note(...), searchSelect(...), text(...), outro(...)) so the module usage is explicit and follows the coding guideline preferring specific imports.packages/cli/src/installers/react-email.ts (1)
6-6: Prefer named imports over namespace imports.Only
textis used from the prompts module in this file, soimport * as padds unnecessary indirection.♻️ Suggested refactor
-import * as p from "~/cli/prompts.js"; +import { text } from "~/cli/prompts.js"; @@ - await p.text({ + await text({ @@ - await p.text({ + await text({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/installers/react-email.ts` at line 6, The current namespace import "import * as p from '~/cli/prompts.js'" introduces indirection while only the text prompt is used; change the import to a named import for text (replace the namespace import with "import { text } from '~/cli/prompts.js'") and update all usages of p.text to text in this module so callers reference the named symbol directly.packages/new/src/utils/http.ts (3)
10-16: Standardize the timeout/options surface across these helpers.This module mixes
timeoutandtimeoutMsfor the same milliseconds value and repeats the10_000literal across every helper. A sharedDEFAULT_TIMEOUT_MSplus one common options type would make the exported API easier to use and keep consistent. As per coding guidelines, "Use meaningful variable names instead of magic numbers - extract constants with descriptive names" and "Use descriptive names for functions, variables, and types."Also applies to: 20-30, 34-40, 46-50, 53-60, 66-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/http.ts` around lines 10 - 16, The helpers (e.g., getJson) use mixed option names (timeout vs timeoutMs) and repeat the literal 10_000; introduce a single exported constant DEFAULT_TIMEOUT_MS and a shared options type (e.g., HttpOptions { headers?: Record<string,string>; timeoutMs?: number }) and update all helper functions (getJson and the other helpers at the cited ranges) to accept that options type and to derive timeout via options?.timeoutMs ?? DEFAULT_TIMEOUT_MS; replace all 10_000 literals with DEFAULT_TIMEOUT_MS and normalize to the timeoutMs name across the module so the public API is consistent.
34-41: Avoid exportingAxiosResponse<any>fromdeleteJson.Because
axios.deleteis not parameterized here and the function has no explicit return type, the public signature falls back toany. MakingdeleteJsongeneric withT = unknownkeeps the helper type-safe for callers.♻️ Suggested typing cleanup
-import axios from "axios"; +import axios, { type AxiosResponse } from "axios"; @@ -export async function deleteJson(url: string, options?: { headers?: Record<string, string>; timeout?: number }) { - const response = await axios.delete(url, { +export async function deleteJson<T = unknown>( + url: string, + options?: { headers?: Record<string, string>; timeout?: number }, +): Promise<AxiosResponse<T>> { + const response = await axios.delete<T>(url, { headers: options?.headers, httpsAgent: createHttpsAgent(), timeout: options?.timeout ?? 10_000, validateStatus: null, }); return response; }As per coding guidelines, "Use explicit types for function parameters and return values when they enhance clarity" and "Prefer
unknownoveranywhen the type is genuinely unknown."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/http.ts` around lines 34 - 41, The exported helper deleteJson currently returns an untyped Axios response (effectively any); make deleteJson generic (e.g., <T = unknown>) and declare an explicit return type of Promise<AxiosResponse<T>> so callers get type-safe response.data typing; update the function signature references to axios.delete<T> and adjust any internal uses to preserve generics (symbols: deleteJson, axios.delete, AxiosResponse, T).
46-50: Allow non-object JSON payloads inrequestJson.
body?: Record<string, unknown>excludes valid JSON payloads like arrays, strings, booleans, andnull, and it is narrower thanpostJsonon Line 22. Widen this tounknownor a dedicatedJsonValuetype so the generic helper can represent the full JSON surface.♻️ Suggested payload typing
options?: { method?: "GET" | "POST" | "DELETE"; headers?: Record<string, string>; - body?: Record<string, unknown>; + body?: unknown; timeoutMs?: number; },As per coding guidelines, "Prefer
unknownoveranywhen the type is genuinely unknown."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/utils/http.ts` around lines 46 - 50, The options.body type in requestJson is too narrow (Record<string, unknown>) and excludes valid JSON payloads (arrays, strings, booleans, null); change the body type to a broader JSON representation (e.g., unknown or a JsonValue union type that covers object, array, string, number, boolean, and null) and update the requestJson signature accordingly so it matches postJson's flexibility; ensure any internal JSON.stringify/use sites in requestJson still handle the widened type safely (with runtime checks if needed) and adjust related types or callers to compile with the new body type.packages/new/src/core/planInit.ts (1)
84-100: Extract the initial-codegen gate into one boolean.This compound condition is duplicated in both
commandsandtasks.runInitialCodegen. Centralizing it will keep those two paths from drifting and makes later edits much easier to audit.As per coding guidelines, "Extract complex conditions into well-named boolean variables".♻️ Suggested extraction
+ const shouldRunInitialCodegen = + request.dataSource === "filemaker" && + !request.skipFileMakerSetup && + !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs); + return { @@ commands: [ ...(request.noInstall ? [] : [{ type: "install" as const }]), - ...(request.dataSource === "filemaker" && - !request.skipFileMakerSetup && - !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs) - ? [{ type: "codegen" as const }] - : []), + ...(shouldRunInitialCodegen ? [{ type: "codegen" as const }] : []), ...(request.noGit ? [] : [{ type: "git-init" as const }]), ], tasks: { bootstrapFileMaker: request.dataSource === "filemaker" && !request.skipFileMakerSetup, runInstall: !request.noInstall, - runInitialCodegen: - request.dataSource === "filemaker" && - !request.skipFileMakerSetup && - !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs), + runInitialCodegen: shouldRunInitialCodegen, initializeGit: !request.noGit, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/new/src/core/planInit.ts` around lines 84 - 100, Introduce a descriptive boolean (e.g., shouldRunInitialCodegen) that captures the compound condition currently duplicated: request.dataSource === "filemaker" && !request.skipFileMakerSetup && !(request.appType === "webviewer" && request.nonInteractive && !request.hasExplicitFileMakerInputs); replace the duplicated expressions used in the commands array spread (the codegen entry) and in tasks.runInitialCodegen with this single boolean, leaving bootstrapFileMaker (request.dataSource === "filemaker" && !request.skipFileMakerSetup) as-is so it remains semantically distinct.
🤖 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/new/src/services/live.ts`:
- Around line 455-538: The spinner (spin) can remain running if startDeployment,
getDeploymentStatus, or createDataAPIKeyWithCredentials reject; wrap the
deployment flow in a try/finally (or ensure spin.stop is called before every
throw) so the spinner is always stopped on error or success: enclose calls to
fileMakerService.startDeployment, the polling loop using
fileMakerService.getDeploymentStatus, and
fileMakerService.createDataAPIKeyWithCredentials in the try block and call
spin.stop(...) in the finally block (or call spin.stop with an appropriate
message immediately before each throw), referencing spin, startDeployment,
getDeploymentStatus, and createDataAPIKeyWithCredentials to locate the code to
change.
- Around line 368-376: The code assumes nested response fields are arrays (e.g.,
the `databases` variable in listFiles() and similar code in listAPIKeys() and
listLayouts()) which can throw when the API returns an object/error; update each
function (listFiles, listAPIKeys, listLayouts) to defensively normalize the
nested response before iterating: check if response.data?.response?.<field> is
an Array with Array.isArray(...) and if so use it, otherwise log or surface the
raw response payload and treat the value as an empty array (or throw a clear
error) before calling .filter() or transformLayoutList(); apply the same
normalization pattern where `databases` (and the layout/apiKeys arrays) are
derived so the real API failure isn’t hidden.
---
Nitpick comments:
In `@packages/cli/src/cli/add/fmschema.ts`:
- Line 8: Replace the namespace import "import * as p from
\"~/cli/prompts.js\";" with a named import that explicitly imports the used
prompt utilities: select, isCancel, cancel, spinner, note, searchSelect, text,
and outro; then update any callsites that reference p.<name> to use the direct
identifiers (e.g., select(...), isCancel(...), cancel(...), spinner(...),
note(...), searchSelect(...), text(...), outro(...)) so the module usage is
explicit and follows the coding guideline preferring specific imports.
In `@packages/cli/src/installers/react-email.ts`:
- Line 6: The current namespace import "import * as p from '~/cli/prompts.js'"
introduces indirection while only the text prompt is used; change the import to
a named import for text (replace the namespace import with "import { text } from
'~/cli/prompts.js'") and update all usages of p.text to text in this module so
callers reference the named symbol directly.
In `@packages/new/src/core/planInit.ts`:
- Around line 84-100: Introduce a descriptive boolean (e.g.,
shouldRunInitialCodegen) that captures the compound condition currently
duplicated: request.dataSource === "filemaker" && !request.skipFileMakerSetup &&
!(request.appType === "webviewer" && request.nonInteractive &&
!request.hasExplicitFileMakerInputs); replace the duplicated expressions used in
the commands array spread (the codegen entry) and in tasks.runInitialCodegen
with this single boolean, leaving bootstrapFileMaker (request.dataSource ===
"filemaker" && !request.skipFileMakerSetup) as-is so it remains semantically
distinct.
In `@packages/new/src/utils/http.ts`:
- Around line 10-16: The helpers (e.g., getJson) use mixed option names (timeout
vs timeoutMs) and repeat the literal 10_000; introduce a single exported
constant DEFAULT_TIMEOUT_MS and a shared options type (e.g., HttpOptions {
headers?: Record<string,string>; timeoutMs?: number }) and update all helper
functions (getJson and the other helpers at the cited ranges) to accept that
options type and to derive timeout via options?.timeoutMs ?? DEFAULT_TIMEOUT_MS;
replace all 10_000 literals with DEFAULT_TIMEOUT_MS and normalize to the
timeoutMs name across the module so the public API is consistent.
- Around line 34-41: The exported helper deleteJson currently returns an untyped
Axios response (effectively any); make deleteJson generic (e.g., <T = unknown>)
and declare an explicit return type of Promise<AxiosResponse<T>> so callers get
type-safe response.data typing; update the function signature references to
axios.delete<T> and adjust any internal uses to preserve generics (symbols:
deleteJson, axios.delete, AxiosResponse, T).
- Around line 46-50: The options.body type in requestJson is too narrow
(Record<string, unknown>) and excludes valid JSON payloads (arrays, strings,
booleans, null); change the body type to a broader JSON representation (e.g.,
unknown or a JsonValue union type that covers object, array, string, number,
boolean, and null) and update the requestJson signature accordingly so it
matches postJson's flexibility; ensure any internal JSON.stringify/use sites in
requestJson still handle the widened type safely (with runtime checks if needed)
and adjust related types or callers to compile with the new body type.
In `@packages/new/src/utils/projectName.ts`:
- Around line 40-55: The error message returned when
VALID_APP_NAME_REGEX.test(scopedAppName ?? "") fails is too narrow; update the
returned string (the literal error message in the function that uses
VALID_APP_NAME_REGEX and scopedAppName/normalized/segments logic) so it
accurately reflects all allowed characters (include ".", "~", and "*" in the
description) or replace it with a simpler, accurate message such as "App name is
not valid."; ensure you update the exact string returned in the branch after
VALID_APP_NAME_REGEX.test to match the regex semantics.
In `@packages/new/tests/project-name.test.ts`:
- Around line 4-23: Add unit tests covering scoped package names: add a test for
parseNameAndPath ensuring input like "apps/@myorg/my-app" returns
["@myorg/my-app","apps/my-app"], and add tests for validateAppName that assert a
valid scoped name (e.g., "@myorg/my-app") returns undefined while an invalid
scope (e.g., "@INVALID/my-app") returns the same validation error string; use
parseNameAndPath and validateAppName in the new cases and keep the test
structure consistent with the existing describe block and vi.restoreAllMocks
setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8438c36d-5c12-4e7e-8632-fc8a1bb0a4e2
📒 Files selected for processing (26)
packages/cli/src/cli/add/data-source/filemaker.tspackages/cli/src/cli/add/fmschema.tspackages/cli/src/cli/add/page/index.tspackages/cli/src/cli/add/registry/install.tspackages/cli/src/cli/ottofms.tspackages/cli/src/cli/prompts.tspackages/cli/src/consts.tspackages/cli/src/helpers/logNextSteps.tspackages/cli/src/installers/proofkit-auth.tspackages/cli/src/installers/proofkit-webviewer.tspackages/cli/src/installers/react-email.tspackages/new/src/consts.tspackages/new/src/core/context.tspackages/new/src/core/planInit.tspackages/new/src/core/resolveInitRequest.tspackages/new/src/index.tspackages/new/src/services/live.tspackages/new/src/utils/browserOpen.tspackages/new/src/utils/http.tspackages/new/src/utils/packageManager.tspackages/new/src/utils/projectName.tspackages/new/src/utils/prompts.tspackages/new/tests/planner.test.tspackages/new/tests/project-name.test.tspackages/new/vitest.config.tspackages/typegen/src/server/createDataApiClient.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cli/src/consts.ts
- packages/new/tests/planner.test.ts
🚧 Files skipped from review as they are similar to previous changes (12)
- packages/cli/src/installers/proofkit-webviewer.ts
- packages/cli/src/cli/add/data-source/filemaker.ts
- packages/cli/src/installers/proofkit-auth.ts
- packages/new/src/utils/packageManager.ts
- packages/new/src/core/resolveInitRequest.ts
- packages/new/vitest.config.ts
- packages/cli/src/cli/add/registry/install.ts
- packages/cli/src/cli/add/page/index.ts
- packages/cli/src/cli/prompts.ts
- packages/new/src/core/context.ts
- packages/new/src/consts.ts
- packages/cli/src/cli/ottofms.ts
Comment on lines
+368
to
+376
| const response = await getJson<{ response?: { databases?: Array<{ filename?: string; status?: string }> } }>( | ||
| `${url.origin}/otto/fmi/admin/api/v2/databases`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${token}`, | ||
| }, | ||
| }, | ||
| ); | ||
| const databases = (response.data?.response?.databases ?? []) as Record<string, unknown>[]; |
There was a problem hiding this comment.
Normalize remote arrays before iterating over them.
listFiles(), listAPIKeys(), and listLayouts() all assume the nested response field is already an array. If Otto/FMS returns an error object or any other unexpected shape, the follow-up .filter() / transformLayoutList() call throws and hides the real API failure.
🛡️ Suggested defensive parsing
- const databases = (response.data?.response?.databases ?? []) as Record<string, unknown>[];
+ const rawDatabases = response.data?.response?.databases;
+ const databases = Array.isArray(rawDatabases) ? rawDatabases : [];
@@
- const apiKeys = (response.data?.response?.["api-keys"] ?? []) as Record<string, unknown>[];
+ const rawApiKeys = response.data?.response?.["api-keys"];
+ const apiKeys = Array.isArray(rawApiKeys) ? rawApiKeys : [];
@@
- return transformLayoutList(response.data?.response?.layouts ?? []);
+ const rawLayouts = response.data?.response?.layouts;
+ return transformLayoutList(Array.isArray(rawLayouts) ? rawLayouts : []);Also applies to: 388-396, 540-549
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/new/src/services/live.ts` around lines 368 - 376, The code assumes
nested response fields are arrays (e.g., the `databases` variable in listFiles()
and similar code in listAPIKeys() and listLayouts()) which can throw when the
API returns an object/error; update each function (listFiles, listAPIKeys,
listLayouts) to defensively normalize the nested response before iterating:
check if response.data?.response?.<field> is an Array with Array.isArray(...)
and if so use it, otherwise log or surface the raw response payload and treat
the value as an empty array (or throw a clear error) before calling .filter() or
transformLayoutList(); apply the same normalization pattern where `databases`
(and the layout/apiKeys arrays) are derived so the real API failure isn’t
hidden.
Comment on lines
+455
to
+538
| const spin = createSpinner(); | ||
| spin.start("Deploying ProofKit Demo file"); | ||
|
|
||
| const deploymentPayload = { | ||
| scheduled: false, | ||
| label: "Install ProofKit Demo", | ||
| deployments: [ | ||
| { | ||
| name: "Install ProofKit Demo", | ||
| source: { | ||
| type: "url", | ||
| url: "https://proofkit.dev/proofkit-demo/manifest.json", | ||
| }, | ||
| fileOperations: [ | ||
| { | ||
| target: { | ||
| fileName: demoFileName, | ||
| }, | ||
| operation, | ||
| source: { | ||
| fileName: demoFileName, | ||
| }, | ||
| location: { | ||
| folder: "default", | ||
| subFolder: "", | ||
| }, | ||
| }, | ||
| ], | ||
| concurrency: 1, | ||
| options: { | ||
| closeFilesAfterBuild: false, | ||
| keepFilesClosedAfterComplete: false, | ||
| transferContainerData: false, | ||
| }, | ||
| }, | ||
| ], | ||
| abortRemaining: false, | ||
| }; | ||
|
|
||
| const deployment = await fileMakerService.startDeployment({ | ||
| payload: deploymentPayload, | ||
| url, | ||
| token, | ||
| }); | ||
|
|
||
| const deploymentId = deployment.data?.response?.subDeploymentIds?.[0]; | ||
| if (!deploymentId) { | ||
| spin.stop("Demo deployment failed"); | ||
| throw new Error("No deployment ID was returned when deploying the demo file."); | ||
| } | ||
|
|
||
| const deploymentDeadline = Date.now() + 300_000; | ||
| let deploymentCompleted = false; | ||
| while (Date.now() < deploymentDeadline) { | ||
| await new Promise((resolve) => setTimeout(resolve, 2500)); | ||
| const status = await fileMakerService.getDeploymentStatus({ | ||
| url, | ||
| token, | ||
| deploymentId, | ||
| }); | ||
|
|
||
| if (!status.data?.response?.running) { | ||
| if (status.data?.response?.status !== "complete") { | ||
| spin.stop("Demo deployment failed"); | ||
| throw new Error("ProofKit Demo deployment did not complete successfully."); | ||
| } | ||
| deploymentCompleted = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!deploymentCompleted) { | ||
| spin.stop("Demo deployment timed out"); | ||
| throw new Error("ProofKit Demo deployment timed out after 5 minutes."); | ||
| } | ||
|
|
||
| const apiKey = await fileMakerService.createDataAPIKeyWithCredentials({ | ||
| url, | ||
| filename: demoFileName, | ||
| username: "admin", | ||
| password: "admin", | ||
| }); | ||
| spin.stop("Demo file deployed"); | ||
| return { apiKey: apiKey.apiKey, filename: demoFileName }; |
There was a problem hiding this comment.
Stop the spinner on thrown deployment errors too.
Only the handled timeout/unsuccessful-status branches call spin.stop(...) today. A rejection from startDeployment(), getDeploymentStatus(), or createDataAPIKeyWithCredentials() exits early with the spinner still running, which muddies the terminal state and error output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/new/src/services/live.ts` around lines 455 - 538, The spinner (spin)
can remain running if startDeployment, getDeploymentStatus, or
createDataAPIKeyWithCredentials reject; wrap the deployment flow in a
try/finally (or ensure spin.stop is called before every throw) so the spinner is
always stopped on error or success: enclose calls to
fileMakerService.startDeployment, the polling loop using
fileMakerService.getDeploymentStatus, and
fileMakerService.createDataAPIKeyWithCredentials in the try block and call
spin.stop(...) in the finally block (or call spin.stop with an appropriate
message immediately before each throw), referencing spin, startDeployment,
getDeploymentStatus, and createDataAPIKeyWithCredentials to locate the code to
change.
Collaborator
Author
eluce2
force-pushed
the
codex/proofkit-new-scaffold
branch
from
0cf5251 to
ed685e7
Compare
This was referenced Mar 19, 2026
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 by CodeRabbit
New Features
Improvements
Tests