test user api#34
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR centralizes dependency versioning with a generated workspace catalog and validator, updates CLI scaffold and dependency-sync to be workspace-aware, enforces dependency-range policies for apps and scaffold sources, adds symbol-keyed cache driver disposal and runtime disposal wiring, and expands unit and integration tests and example manifests. ChangesDependency Version Centralization & Cache Disposal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 6
🧹 Nitpick comments (3)
packages/cli/src/project/registry.ts (1)
87-91: ⚡ Quick winOptimize the existence check.
The code reads the entire file just to check if it exists, then discards the result. A more efficient approach would be to either:
- Use
fs.access()to check file existence- Let
importProjectModulehandle the error and catch it⚡ Proposed optimization
Option 1: Use fs.access()
+import { access } from 'node:fs/promises' async function renderGeneratedSchemaRuntimeArtifact( projectRoot: string, schemaEntry: string, ): Promise<string> { const schemaPath = resolve(projectRoot, schemaEntry) try { - await readFile(schemaPath, 'utf8') + await access(schemaPath) } catch { return renderGeneratedSchemaRuntimeModule([]) } const moduleValue = await importProjectModule(projectRoot, schemaPath) const tables = extractGeneratedSchemaTables(moduleValue) return renderGeneratedSchemaRuntimeModule(tables) }Option 2: Catch importProjectModule errors
async function renderGeneratedSchemaRuntimeArtifact( projectRoot: string, schemaEntry: string, ): Promise<string> { const schemaPath = resolve(projectRoot, schemaEntry) - try { - await readFile(schemaPath, 'utf8') - } catch { - return renderGeneratedSchemaRuntimeModule([]) - } - const moduleValue = await importProjectModule(projectRoot, schemaPath) + const moduleValue = await importProjectModule(projectRoot, schemaPath).catch(() => undefined) + if (!moduleValue) { + return renderGeneratedSchemaRuntimeModule([]) + } const tables = extractGeneratedSchemaTables(moduleValue) return renderGeneratedSchemaRuntimeModule(tables) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/project/registry.ts` around lines 87 - 91, The current code uses readFile(schemaPath, 'utf8') just to test existence then discards the result; replace that with a proper existence check or let the module import handle it: either call fs.promises.access(schemaPath, fs.constants.R_OK) in place of readFile and catch the error to return renderGeneratedSchemaRuntimeModule([]), or remove the try/catch and let importProjectModule(...) handle missing-file errors and catch them where importProjectModule is invoked; update references to readFile(schemaPath, 'utf8') accordingly and keep renderGeneratedSchemaRuntimeModule([]) as the fallback.packages/cli/src/generators.ts (1)
66-82: 💤 Low valueConsider the limitations of string-based duplicate detection.
The
findGeneratedModelSourceByTableNamehelper uses a simple.includes()check to find models with matching table names. While this works for typical cases, it has known limitations:
- False positives: Comments, string literals, or formatted code containing the pattern
- False negatives: Aliased imports (
import { defineModel as dm }), template literals, or variables for table namesFor a CLI scaffolding tool working with conventionally structured code, this heuristic is likely sufficient. However, if you encounter edge cases in practice, consider using an AST-based approach (e.g., with
ast-grepor TypeScript compiler API).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/generators.ts` around lines 66 - 82, The current findGeneratedModelSourceByTableName uses a raw string .includes(generatedTableReference) which can yield false positives/negatives; update it to either (1) pre-process file contents to strip comments and string/template literal boundaries and then search with a stricter regex like /defineModel\s*\(\s*(['"`])<tableName>\1/ (use the generatedTableReference variable for the tableName) or (2) replace the heuristic with an AST scan: parse files (e.g., with `@babel/parser` or TypeScript compiler API), traverse for CallExpression nodes whose callee identifier is defineModel (or an imported alias) and whose first argument is a literal matching tableName; adjust collectFiles/readFile usage and return basename as before.packages/security/tests/real-redis.test.ts (1)
21-35: ⚡ Quick winAdd an explicit post-threshold assertion for rate-limit enforcement.
This test validates counter increments but doesn’t assert behavior once the max-attempts boundary is exceeded. Adding one more
hitand assertinglimited === truewould close that gap.💡 Suggested test extension
const secondHit = await store.hit('login:127.0.0.1', { maxAttempts: 2, decaySeconds: 60, }) + const thirdHit = await store.hit('login:127.0.0.1', { + maxAttempts: 2, + decaySeconds: 60, + }) const cleared = await store.clear('login:127.0.0.1') expect(firstHit.limited).toBe(false) expect(firstHit.snapshot.attempts).toBe(1) expect(secondHit.limited).toBe(false) expect(secondHit.snapshot.attempts).toBe(2) + expect(thirdHit.limited).toBe(true) expect(cleared).toBe(true)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/security/tests/real-redis.test.ts` around lines 21 - 35, The test currently checks two successful calls to store.hit and then store.clear but doesn't assert behavior when the maxAttempts boundary is exceeded; add one more call to store.hit('login:127.0.0.1', { maxAttempts: 2, decaySeconds: 60 }) after the secondHit and before cleared, and assert that the returned object's limited property is true (and optionally snapshot.attempts reflects the limit) to verify rate-limit enforcement for store.hit and ensure store.clear is still tested afterwards.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/adapter-next/package.json`:
- Around line 45-47: Replace the workspace-local specifier "next": "catalog:" in
package.json with an explicit semver range for Next.js (e.g., "^13.0.0 ||
^14.0.0") so the package's peer/dependencies are valid for publishing; locate
the "next": "catalog:" entry in packages/adapter-next's package.json (the "next"
key) and update it to the chosen semver range, ensuring it is listed under the
appropriate dependencies/peerDependencies field as intended for consumers.
In `@packages/cache/src/redis.ts`:
- Around line 149-166: The dispose method can race with an in-flight
initialization (this.pending) causing a disposed driver to be cached; add a
disposal marker (e.g., a boolean flag or a symbol like
CACHE_DRIVER_DISPOSED_SYMBOL) that you set inside CACHE_DRIVER_DISPOSE_SYMBOL()
when clearing this.pending/this.driverInstance, and then update the pending
resolution path (the code that currently assigns this.driverInstance when the
pending promise resolves) to check that the disposal marker is not set before
setting this.driverInstance; also clear or reset the marker appropriately so
subsequent initializations still work and keep using the DisposableCacheDriver
symbol for calling disposables.
In `@packages/cache/src/runtime-shared.ts`:
- Around line 44-52: In disposeCacheRuntimeBindings, guard each call to
disposeDriver so a thrown error from one driver doesn't abort the loop: iterate
over bindings.drivers.values() and wrap disposeDriver(driver) in a try/catch
that logs or records the error (but continues disposing remaining drivers);
ensure the function still returns void and that the catch references the driver
identity if possible to aid debugging.
In `@packages/cli/src/metadata.ts`:
- Around line 1-2: The root package.json import (workspacePackageJson) in
metadata.ts cannot be resolved for npm consumers; replace that out-of-package
import by switching metadata.ts to read catalog values from a build-generated
constants module (e.g., src/generated/workspaceCatalog.ts) produced during your
build step, and update all exports that currently reference workspacePackageJson
(ESBUILD_PACKAGE_VERSION, IOREDIS_PACKAGE_VERSION, and all SCAFFOLD_*_VERSIONS)
to use the generated constants instead; alternatively, if you prefer bundling,
configure tsup/esbuild to inline JSON imports so the '../../../package.json' is
inlined, but do not keep the raw relative import in the published package.
In `@packages/cli/src/project/scaffold/framework.ts`:
- Around line 92-95: The scaffolded package manifest omits eslint from the
devDependencies so the generated lint scripts (which call eslint directly) will
fail; update the devDependencies object (the const devDependencies in
framework.ts) to include 'eslint' with the version taken from
SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS['eslint'] (and similarly add any related
lint packages you expect to ship, e.g., '@typescript-eslint/eslint-plugin' or
'@typescript-eslint/parser' if referenced by generated configs) so the
scaffolded project has eslint installed by default.
In `@scripts/validate-dependency-version-policy.mjs`:
- Around line 70-85: The current collectScaffoldSourceFailures check only
inspects the literal text inside renderScaffoldPackageJson and misses forbidden
ranges moved to imported constants; update the check to also detect forbidden
dependency ranges referenced via imports by (1) parsing import statements in
packages/cli/src/project/scaffold/framework.ts for any identifiers used inside
renderScaffoldPackageJson, (2) resolving those imported module files and reading
their exported constant values, and (3) scanning those resolved values for any
of the forbiddenRanges (['workspace:', 'catalog:']); ensure changes reference
collectScaffoldSourceFailures, renderScaffoldPackageJson, scaffoldPath and
forbiddenRanges so the validator flags both inline literals and values coming
from imports.
---
Nitpick comments:
In `@packages/cli/src/generators.ts`:
- Around line 66-82: The current findGeneratedModelSourceByTableName uses a raw
string .includes(generatedTableReference) which can yield false
positives/negatives; update it to either (1) pre-process file contents to strip
comments and string/template literal boundaries and then search with a stricter
regex like /defineModel\s*\(\s*(['"`])<tableName>\1/ (use the
generatedTableReference variable for the tableName) or (2) replace the heuristic
with an AST scan: parse files (e.g., with `@babel/parser` or TypeScript compiler
API), traverse for CallExpression nodes whose callee identifier is defineModel
(or an imported alias) and whose first argument is a literal matching tableName;
adjust collectFiles/readFile usage and return basename as before.
In `@packages/cli/src/project/registry.ts`:
- Around line 87-91: The current code uses readFile(schemaPath, 'utf8') just to
test existence then discards the result; replace that with a proper existence
check or let the module import handle it: either call
fs.promises.access(schemaPath, fs.constants.R_OK) in place of readFile and catch
the error to return renderGeneratedSchemaRuntimeModule([]), or remove the
try/catch and let importProjectModule(...) handle missing-file errors and catch
them where importProjectModule is invoked; update references to
readFile(schemaPath, 'utf8') accordingly and keep
renderGeneratedSchemaRuntimeModule([]) as the fallback.
In `@packages/security/tests/real-redis.test.ts`:
- Around line 21-35: The test currently checks two successful calls to store.hit
and then store.clear but doesn't assert behavior when the maxAttempts boundary
is exceeded; add one more call to store.hit('login:127.0.0.1', { maxAttempts: 2,
decaySeconds: 60 }) after the secondHit and before cleared, and assert that the
returned object's limited property is true (and optionally snapshot.attempts
reflects the limit) to verify rate-limit enforcement for store.hit and ensure
store.clear is still tested afterwards.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4b802564-d5da-4752-b7ad-0d3576a93b34
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
README.mdapps/blog-next/package.jsonapps/blog-nuxt/package.jsonapps/blog-sveltekit/package.jsonpackage.jsonpackages/adapter-next/package.jsonpackages/adapter-next/tests/adapter.type.test.tspackages/auth/package.jsonpackages/cache-redis/src/index.tspackages/cache-redis/tests/package.test.tspackages/cache-redis/tests/real-redis.test.tspackages/cache/src/redis.tspackages/cache/src/runtime-shared.tspackages/cache/src/runtime.tspackages/cache/tests/package.test.tspackages/cli/src/cli.tspackages/cli/src/generators.tspackages/cli/src/metadata.tspackages/cli/src/project/discovery-helpers.tspackages/cli/src/project/registry-svelte.tspackages/cli/src/project/registry.tspackages/cli/src/project/scaffold/dependencies.tspackages/cli/src/project/scaffold/framework-renderers.tspackages/cli/src/project/scaffold/framework.tspackages/cli/tests/cli.test.tspackages/core/tests/optional-peers.published.test.tspackages/db-mysql/tests/mysql.test.tspackages/db-postgres/tests/postgres.test.tspackages/db-sqlite/tests/sqlite.test.tspackages/queue-redis/tests/real-redis.test.tspackages/security/tests/real-redis.test.tspackages/session/tests/real-redis.test.tsscripts/validate-dependency-version-policy.mjs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/generate-cli-workspace-catalog.mjs (1)
13-22: ⚡ Quick winCreate the target directory before writing the generated file.
Line 14 assumes
packages/cli/src/generatedalready exists.writeFilewill throwENOENTif that directory is missing, which makes the generator fragile for clean clones or after cleanup.Suggested hardening
-import { readFile, writeFile } from 'node:fs/promises' +import { mkdir, readFile, writeFile } from 'node:fs/promises' import { dirname, join, resolve } from 'node:path' import { fileURLToPath } from 'node:url' @@ +const outputPath = join(repoRoot, 'packages/cli/src/generated/workspaceCatalog.ts') +await mkdir(dirname(outputPath), { recursive: true }) + await writeFile( - join(repoRoot, 'packages/cli/src/generated/workspaceCatalog.ts'), + outputPath, [ '// Generated by scripts/generate-cli-workspace-catalog.mjs. Do not edit by hand.',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/generate-cli-workspace-catalog.mjs` around lines 13 - 22, The script attempts to write the generated file directly and will throw ENOENT if the target directory doesn't exist; before calling writeFile for the path built with join(repoRoot, 'packages/cli/src/generated', 'workspaceCatalog.ts'), ensure the containing directory is created (use fs.promises.mkdir or mkdirSync with { recursive: true } on the directory name derived from the join result or via path.dirname) and await it, then proceed to writeFile; reference the existing join, writeFile and repoRoot identifiers when adding the mkdir step so the folder is created reliably on clean clones.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/project/registry.ts`:
- Around line 832-845: The current JSON parse catch in the registry loader
returns undefined on failure, which prevents falling back to the generated/index
module; change the behavior so a malformed generated-registry.json does NOT
short-circuit the method: in the try/catch around JSON.parse(contents)
(variables: contents, parsed, isGeneratedProjectRegistry) remove the immediate
return in the catch and instead allow execution to continue to the fallback
logic that builds generatedIndexPath (using GENERATED_INDEX_PATH) and calls
importProjectModule(projectRoot, generatedIndexPath); optionally record or log
the parse error before continuing, but ensure the import fallback always runs
when JSON is invalid.
- Around line 88-92: The current try/catch around await access(schemaPath,
fsConstants.R_OK) swallows all filesystem errors; change the catch to only
handle missing-file cases by inspecting the error.code and returning
renderGeneratedSchemaRuntimeModule([]) when code is 'ENOENT' or 'ENOTDIR', and
re-throw the error for any other codes (e.g., 'EACCES', 'EPERM', etc.); locate
the access call and surrounding block in registry.ts and replace the blanket
catch with a conditional that checks err.code before deciding to return the
empty module or throw the original error.
In `@scripts/validate-dependency-version-policy.mjs`:
- Around line 70-89: collectImportBindings currently only captures named imports
via importPattern and misses default (import X from "...") and namespace imports
(import * as X from "..."), so extend import handling: either replace the
fragile importPattern with a proper JS parser (e.g.,
acorn/espree/es-module-lexer) to reliably extract ImportDeclaration nodes, or
expand the regex logic in collectImportBindings to also match default imports
and namespace imports and map them into bindings (use importedName 'default' for
default imports and '*' for namespace imports, keeping localName as the binding
key). Also update the corresponding function-detection logic (the code that
currently only finds function declarations — refer to the function that performs
recursive validation of function bodies) to also detect arrow functions and
variable-assigned functions (const/let/var x = () => or x = function() ) by
using the same parser or additional patterns so imported constants and arrow
functions are tracked in the recursive validation. Ensure bindings map entries
use consistent shapes ({ importedName, sourcePath }) so downstream validation of
catalog:/workspace: ranges will see these additional import types.
---
Nitpick comments:
In `@scripts/generate-cli-workspace-catalog.mjs`:
- Around line 13-22: The script attempts to write the generated file directly
and will throw ENOENT if the target directory doesn't exist; before calling
writeFile for the path built with join(repoRoot, 'packages/cli/src/generated',
'workspaceCatalog.ts'), ensure the containing directory is created (use
fs.promises.mkdir or mkdirSync with { recursive: true } on the directory name
derived from the join result or via path.dirname) and await it, then proceed to
writeFile; reference the existing join, writeFile and repoRoot identifiers when
adding the mkdir step so the folder is created reliably on clean clones.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b04d916a-77f1-4c8c-aa30-840302cee5db
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.lockpackages/cli/src/generated/workspaceCatalog.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
package.jsonpackages/adapter-next/package.jsonpackages/cache/src/redis.tspackages/cache/src/runtime-shared.tspackages/cache/tests/package.test.tspackages/cli/package.jsonpackages/cli/src/generators.tspackages/cli/src/metadata.tspackages/cli/src/project/registry.tspackages/cli/src/project/scaffold/framework.tspackages/cli/tests/cli.test.tspackages/security/tests/real-redis.test.tsscripts/generate-cli-workspace-catalog.mjsscripts/validate-dependency-version-policy.mjsscripts/validate-dependency-version-policy.test.mjs
✅ Files skipped from review due to trivial changes (2)
- package.json
- packages/adapter-next/package.json
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/cache/src/redis.ts
- packages/cache/src/runtime-shared.ts
- packages/security/tests/real-redis.test.ts
- packages/cli/src/project/scaffold/framework.ts
- packages/cli/src/generators.ts
- packages/cli/tests/cli.test.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
scripts/validate-dependency-version-policy.test.mjs (3)
8-121: 💤 Low valueOptional: Extract common test setup to reduce duplication.
The three tests share significant structural code (directory creation, file writing pattern, assertion structure). You could extract a helper function to reduce boilerplate while maintaining test clarity.
♻️ Example helper extraction
async function createTestScaffold(files) { const repoRoot = await mkdtemp(join(tmpdir(), 'holo-dependency-policy-')) await mkdir(join(repoRoot, 'packages/cli/src/project/scaffold'), { recursive: true }) await mkdir(join(repoRoot, 'packages/cli/src'), { recursive: true }) for (const [path, content] of Object.entries(files)) { await writeFile(join(repoRoot, path), content.join('\n'), 'utf8') } return repoRoot } test('dependency policy validator catches forbidden scaffold ranges from imported constants', async () => { const repoRoot = await createTestScaffold({ 'packages/cli/src/metadata.ts': [ 'export const SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS = Object.freeze({', ' eslint: \'catalog:\',', '} as const)', '', ], 'packages/cli/src/project/scaffold/framework.ts': [ 'import { SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS } from \'../../metadata\'', '', 'export function renderScaffoldPackageJson() {', ' const devDependencies = {', ' eslint: SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS.eslint,', ' }', ' return JSON.stringify({ devDependencies })', '}', '', 'export async function scaffoldProject() {}', '', ], }) const failures = await collectScaffoldSourceFailures(repoRoot) assert.equal(failures.length, 1) assert.match(failures[0], /catalog:/) assert.match(failures[0], /SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS/) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate-dependency-version-policy.test.mjs` around lines 8 - 121, Tests duplicate scaffold setup across test cases; extract a helper to reduce boilerplate. Create a helper (e.g., createTestScaffold) that makes the temp repo, creates the packages/cli/src and packages/cli/src/project/scaffold directories, and writes provided file contents from a files map; then update the three tests (those invoking collectScaffoldSourceFailures and asserting on failures) to call createTestScaffold with the appropriate file entries instead of repeating mkdir/writeFile logic, keeping assertions and references to SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS, renderScaffoldPackageJson, and collectScaffoldSourceFailures unchanged.
8-121: ⚡ Quick winConsider adding a positive test case.
All three tests verify that the validator correctly catches forbidden
catalog:ranges. Consider adding at least one test that verifies the validator allows valid version strings (e.g.,"^1.0.0","~2.3.4") and produces zero failures, ensuring there are no false positives.🧪 Example positive test case
test('dependency policy validator allows valid semver ranges', async () => { const repoRoot = await mkdtemp(join(tmpdir(), 'holo-dependency-policy-')) await mkdir(join(repoRoot, 'packages/cli/src/project/scaffold'), { recursive: true }) await mkdir(join(repoRoot, 'packages/cli/src'), { recursive: true }) await writeFile(join(repoRoot, 'packages/cli/src/metadata.ts'), [ 'export const SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS = Object.freeze({', ' eslint: \'^8.0.0\',', '} as const)', '', ].join('\n'), 'utf8') await writeFile(join(repoRoot, 'packages/cli/src/project/scaffold/framework.ts'), [ 'import { SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS } from \'../../metadata\'', '', 'export function renderScaffoldPackageJson() {', ' const devDependencies = {', ' eslint: SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS.eslint,', ' }', ' return JSON.stringify({ devDependencies })', '}', '', 'export async function scaffoldProject() {}', '', ].join('\n'), 'utf8') const failures = await collectScaffoldSourceFailures(repoRoot) assert.equal(failures.length, 0, 'Valid semver ranges should not produce failures') })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate-dependency-version-policy.test.mjs` around lines 8 - 121, Add a positive test that asserts the validator does not flag valid semver ranges: create a new test (e.g., "dependency policy validator allows valid semver ranges") following the same scaffold setup pattern used in the other tests, write a metadata.ts that sets SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS (or a default export) to a valid semver like '^8.0.0', write the matching framework.ts that imports SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS and uses it for devDependencies, call collectScaffoldSourceFailures(repoRoot) and assert.equal(failures.length, 0) to ensure no failures are reported. Ensure the test references the same symbols (SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS, framework.ts, collectScaffoldSourceFailures) so it mirrors the existing tests.
9-9: ⚡ Quick winConsider adding cleanup for temporary directories.
The tests create temporary directories but don't clean them up. While the OS typically cleans the temp directory periodically, explicitly removing test artifacts improves resource management and prevents accumulation during frequent test runs.
♻️ Add cleanup using Node.js test hooks
You can add cleanup at the end of each test:
test('dependency policy validator catches forbidden scaffold ranges from imported constants', async () => { const repoRoot = await mkdtemp(join(tmpdir(), 'holo-dependency-policy-')) + + try { await mkdir(join(repoRoot, 'packages/cli/src/project/scaffold'), { recursive: true }) // ... rest of test assert.equal(failures.length, 1) assert.match(failures[0], /catalog:/) assert.match(failures[0], /SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS/) + } finally { + await rm(repoRoot, { recursive: true, force: true }) + } })Don't forget to add the import at the top:
-import { mkdir, mkdtemp, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'Also applies to: 40-40, 81-81
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/validate-dependency-version-policy.test.mjs` at line 9, The tests create temporary directories via mkdtemp(join(tmpdir(), 'holo-dependency-policy-')) and assign them to repoRoot (and similar vars at the other locations) but never remove them; add cleanup using Node's test hooks (e.g., test.afterEach or afterEach) to call fs.rm or fs.promises.rm on the created temp dirs with { recursive: true, force: true } to remove repoRoot (and the temp dirs at the other occurrences around lines 40 and 81) after each test finishes; ensure you import/require fs.promises.rm (or fs.rm) if not already imported.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/tests/cli.test.ts`:
- Around line 14917-14921: The test currently only collects top-level dist/*.mjs
into publishedEntrypoints, missing the CLI bin file; update the collection to
also read the CLI bin entrypoint (publishedBin) from join(built.cliPackageRoot,
'dist', 'bin', 'holo.mjs') (using readFile) and include its contents when
building the publishedEntrypoints array or include publishedBin in the
subsequent publish assertion that checks for workspace-artifact leaks; reference
the variables publishedEntrypoints, publishedBin, built.cliPackageRoot, and the
readdir/readFile logic to locate where to add this extra file.
- Around line 14656-14695: The test "propagates generated schema access errors
other than missing files" currently uses chmod(...) to force an EACCES which is
unreliable on Windows; replace the chmod-based permission manipulation with a
vitest mock that causes the file read for
'.holo-js/generated/schema.generated.ts' to throw an error with code 'EACCES'
when writeGeneratedProjectRegistry is invoked. Specifically, in this test
(created via createTempProject and calling writeGeneratedProjectRegistry) remove
the chmod/join/chmod restore calls and instead use vi.spyOn(fs, 'readFile' or
'promises.readFile' depending on the implementation) or vi.mock to intercept
reads of the path '.holo-js/generated/schema.generated.ts' and throw an Error
with code='EACCES', ensure the mock is restored in the finally block, and keep
the expect(...).rejects.toMatchObject({ code: 'EACCES' }) assertion unchanged to
verify propagation.
In `@scripts/generate-cli-workspace-catalog.test.mjs`:
- Line 24: The test's assertion only checks for the substring "eslint" in
output; strengthen it by asserting that "eslint" appears as a property or part
of the actual generated structure rather than incidental text: update the
assertion on the variable output in
scripts/generate-cli-workspace-catalog.test.mjs to match a pattern that verifies
a property (e.g. use a regex such as /"eslint"\s*:/ to check JSON output or a
module-export pattern that includes eslint) or assert that the expected
export/container (e.g. a generated workspace catalog export name) contains an
eslint entry.
- Around line 8-25: The test creates a temp dir with mkdtemp(tmpdir(), ...)
stored in repoRoot but never removes it; update the test file
(scripts/generate-cli-workspace-catalog.test.mjs) to declare let repoRoot in the
outer scope, assign it inside the test that calls generateCliWorkspaceCatalog,
and add an after() hook that checks repoRoot and removes it (e.g., using
fs.promises.rm or fs.promises.rmdir with recursive: true) to clean up the
temporary directory; ensure the cleanup runs even if the test fails so no temps
accumulate.
---
Nitpick comments:
In `@scripts/validate-dependency-version-policy.test.mjs`:
- Around line 8-121: Tests duplicate scaffold setup across test cases; extract a
helper to reduce boilerplate. Create a helper (e.g., createTestScaffold) that
makes the temp repo, creates the packages/cli/src and
packages/cli/src/project/scaffold directories, and writes provided file contents
from a files map; then update the three tests (those invoking
collectScaffoldSourceFailures and asserting on failures) to call
createTestScaffold with the appropriate file entries instead of repeating
mkdir/writeFile logic, keeping assertions and references to
SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS, renderScaffoldPackageJson, and
collectScaffoldSourceFailures unchanged.
- Around line 8-121: Add a positive test that asserts the validator does not
flag valid semver ranges: create a new test (e.g., "dependency policy validator
allows valid semver ranges") following the same scaffold setup pattern used in
the other tests, write a metadata.ts that sets
SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS (or a default export) to a valid semver
like '^8.0.0', write the matching framework.ts that imports
SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS and uses it for devDependencies, call
collectScaffoldSourceFailures(repoRoot) and assert.equal(failures.length, 0) to
ensure no failures are reported. Ensure the test references the same symbols
(SCAFFOLD_BASE_DEV_DEPENDENCY_VERSIONS, framework.ts,
collectScaffoldSourceFailures) so it mirrors the existing tests.
- Line 9: The tests create temporary directories via mkdtemp(join(tmpdir(),
'holo-dependency-policy-')) and assign them to repoRoot (and similar vars at the
other locations) but never remove them; add cleanup using Node's test hooks
(e.g., test.afterEach or afterEach) to call fs.rm or fs.promises.rm on the
created temp dirs with { recursive: true, force: true } to remove repoRoot (and
the temp dirs at the other occurrences around lines 40 and 81) after each test
finishes; ensure you import/require fs.promises.rm (or fs.rm) if not already
imported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 307046fe-a1ac-4bb6-87b2-235281ea3835
📒 Files selected for processing (6)
packages/cli/src/project/registry.tspackages/cli/tests/cli.test.tsscripts/generate-cli-workspace-catalog.mjsscripts/generate-cli-workspace-catalog.test.mjsscripts/validate-dependency-version-policy.mjsscripts/validate-dependency-version-policy.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- scripts/generate-cli-workspace-catalog.mjs
- scripts/validate-dependency-version-policy.mjs
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Merged
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
Bug Fixes
Tests
Documentation