Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds jiti fallback for resolving plugin modules from bare package specifiers, changes plugin module resolution to derive the load directory from the schema file that declares the plugin, introduces a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/actions/migrate.ts (1)
50-58:⚠️ Potential issue | 🟠 MajorMove schema generation into
runDiffor make it conditional for non-schema-based diff modes.The
run()function unconditionally callsrequireDataSourceUrl()andgenerateTempPrismaSchema()at lines 52-55 before the switch statement. This will fail for valid diff modes like URL-to-URL or migrations-to-migrations that don't require the current project's schema. The temp schema should only be generated whenfromSchemaDatamodelortoSchemaDatamodelis set to a schema-based mode, not for datasource-to-datasource comparisons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/actions/migrate.ts` around lines 50 - 58, The current run() unconditionally calls requireDataSourceUrl(schemaFile) and generateTempPrismaSchema(schemaFile, prismaSchemaDir), which breaks URL-to-URL and migrations-only diff modes; move schema validation/generation into runDiff or guard it with checks on the diff modes so a temp schema is only created when a schema-based side is required (e.g., when fromSchemaDatamodel or toSchemaDatamodel equals a schema mode). Concretely, remove or defer the requireDataSourceUrl/schema generation from run(), and in runDiff (or the call site that inspects diff mode) call requireDataSourceUrl and generateTempPrismaSchema only when fromSchemaDatamodel === 'schema' or toSchemaDatamodel === 'schema' (or whichever enum/value you use for schema-based modes), preserving the existing prismaSchemaDir logic when constructing the temp schema.
🧹 Nitpick comments (2)
packages/cli/test/migrate.test.ts (1)
73-76: Strengthen this test beyond “no throw”.This currently behaves as a smoke test only. If
runCliexposes stdout/stderr, assert the--scriptoutput contains expected SQL diff content to catch functional regressions inmigrate diff.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/test/migrate.test.ts` around lines 73 - 76, The test only checks that migrate diff doesn't throw; enhance it by capturing runCli's stdout/stderr and asserting the --script output contains expected SQL diff text (e.g., "CREATE TABLE", table/column names from the provided model, or specific ALTER/CREATE statements) to validate the generated SQL; update the test case in migrate.test.ts (the "supports migrate diff with --from-empty and --to-schema-datamodel" it block) to call runCli in a way that returns stdout/stderr and add assertions against that output rather than relying solely on no exception.packages/cli/src/actions/generate.ts (1)
189-193: Consider extracting shared plugin source-path resolution logic.This logic now exists in both
generate.tsandcheck.ts. Moving it toaction-utils.tswould reduce drift risk and keep plugin resolution behavior centralized.Refactor sketch
+// action-utils.ts +export function getPluginSourcePath(plugin: { $cstNode?: any }, schemaFile: string) { + return plugin.$cstNode?.parent?.element.$document?.uri?.fsPath ?? schemaFile; +}-const pluginSourcePath = - plugin.$cstNode?.parent?.element.$document?.uri?.fsPath ?? schemaFile; +const pluginSourcePath = getPluginSourcePath(plugin, schemaFile);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/actions/generate.ts` around lines 189 - 193, Extract the repeated plugin source-path resolution into a shared helper in action-utils.ts (e.g., resolvePluginSourceDir or getPluginSourceDir) that accepts a plugin node and a fallback schemaFile and returns the directory path (using plugin.$cstNode?.parent?.element.$document?.uri?.fsPath ?? schemaFile then path.dirname(...)); replace the inlined logic in generate.ts (where cliPlugin is loaded with loadPluginModule(provider, path.dirname(pluginSourcePath))) and the similar code in check.ts to call the new helper and pass its result into loadPluginModule so both places share the same resolution behavior.
🤖 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/actions/migrate.ts`:
- Around line 162-205: runDiff constructs a shell command by joining parts and
calls execPrisma with a single string, which allows unescaped values
(options.fromUrl, options.toUrl, options.shadowDatabaseUrl, options.extraArgs)
to be interpreted by the shell; change execPrisma usage so arguments are passed
safely (either by making execPrisma accept an argv array and call
child_process.spawn/execFile without a shell, or by properly shell-escaping each
part before joining) and update runDiff to pass the array of parts (or escaped
parts) instead of parts.join(' '); reference runDiff and execPrisma (and the
exec-utils helper that currently uses execSync) when making the change.
In `@packages/sdk/src/prisma/prisma-schema-generator.ts`:
- Around line 556-590: The FK generation uses ID fields from oppositeModel and
only the first ID, causing wrong references and dropping composite keys; update
the logic in prisma-schema-generator to call getIdFields(decl) (not
oppositeModel), iterate all returned id field names to (a) create corresponding
ref FK fields on prismaOppositeModel (using ModelFieldType and allFields to
mirror types) and (b) build the `@relation` 'fields' and 'references' arrays by
mapping each idFieldName to a PrismaAttributeArgValue('FieldReference', new
PrismaFieldReference(...)) — use fieldName+CapitalizedId for the FK names and
idFieldName for references so addField(fieldName, new
ModelFieldType(decl.name,...), [new PrismaFieldAttribute('@relation', [...])])
contains arrays for all id fields rather than a single idFields[0].
- Around line 542-555: In generateMissingOppositeRelations(), the opposite-field
lookup currently matches only by target model name (using
getAllFields(...).filter(f => f.type.reference?.ref?.name === decl.name));
change this to extract the `@relation` name from the field (e.g., read relation
name from the field's relation metadata—refer to symbols fieldRel / thisRelation
used by the validator) and match opposite fields by relation identity (relation
name) in addition to the target model so multiple relations to the same model
are distinguished; update the filter to compare the extracted relation name for
both the current field and each candidate opposite field. Also replace the
silent continue when a generated fieldName collides with an existing
prismaOppositeModel field (the prismaOppositeModel.fields.some check) with
explicit error handling (throw or processLogger.error + throw) so name
collisions are surfaced instead of being skipped.
---
Outside diff comments:
In `@packages/cli/src/actions/migrate.ts`:
- Around line 50-58: The current run() unconditionally calls
requireDataSourceUrl(schemaFile) and generateTempPrismaSchema(schemaFile,
prismaSchemaDir), which breaks URL-to-URL and migrations-only diff modes; move
schema validation/generation into runDiff or guard it with checks on the diff
modes so a temp schema is only created when a schema-based side is required
(e.g., when fromSchemaDatamodel or toSchemaDatamodel equals a schema mode).
Concretely, remove or defer the requireDataSourceUrl/schema generation from
run(), and in runDiff (or the call site that inspects diff mode) call
requireDataSourceUrl and generateTempPrismaSchema only when fromSchemaDatamodel
=== 'schema' or toSchemaDatamodel === 'schema' (or whichever enum/value you use
for schema-based modes), preserving the existing prismaSchemaDir logic when
constructing the temp schema.
---
Nitpick comments:
In `@packages/cli/src/actions/generate.ts`:
- Around line 189-193: Extract the repeated plugin source-path resolution into a
shared helper in action-utils.ts (e.g., resolvePluginSourceDir or
getPluginSourceDir) that accepts a plugin node and a fallback schemaFile and
returns the directory path (using
plugin.$cstNode?.parent?.element.$document?.uri?.fsPath ?? schemaFile then
path.dirname(...)); replace the inlined logic in generate.ts (where cliPlugin is
loaded with loadPluginModule(provider, path.dirname(pluginSourcePath))) and the
similar code in check.ts to call the new helper and pass its result into
loadPluginModule so both places share the same resolution behavior.
In `@packages/cli/test/migrate.test.ts`:
- Around line 73-76: The test only checks that migrate diff doesn't throw;
enhance it by capturing runCli's stdout/stderr and asserting the --script output
contains expected SQL diff text (e.g., "CREATE TABLE", table/column names from
the provided model, or specific ALTER/CREATE statements) to validate the
generated SQL; update the test case in migrate.test.ts (the "supports migrate
diff with --from-empty and --to-schema-datamodel" it block) to call runCli in a
way that returns stdout/stderr and add assertions against that output rather
than relying solely on no exception.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 218dfde8-e071-42b3-b015-88e6eca927af
📒 Files selected for processing (12)
packages/cli/src/actions/action-utils.tspackages/cli/src/actions/check.tspackages/cli/src/actions/generate.tspackages/cli/src/actions/migrate.tspackages/cli/src/index.tspackages/cli/test/generate.test.tspackages/cli/test/migrate.test.tspackages/cli/test/plugins/prisma-plugin.test.tspackages/language/res/stdlib.zmodelpackages/language/src/validators/datamodel-validator.tspackages/sdk/src/prisma/prisma-builder.tspackages/sdk/src/prisma/prisma-schema-generator.ts
Comment on lines
+162
to
+205
| function runDiff(prismaSchemaFile: string, options: DiffOptions) { | ||
| try { | ||
| const parts = ['migrate diff']; | ||
|
|
||
| if (options.fromEmpty) { | ||
| parts.push('--from-empty'); | ||
| } | ||
| if (options.toEmpty) { | ||
| parts.push('--to-empty'); | ||
| } | ||
| if (options.fromSchemaDatamodel) { | ||
| parts.push(`--from-schema-datamodel "${prismaSchemaFile}"`); | ||
| } | ||
| if (options.toSchemaDatamodel) { | ||
| parts.push(`--to-schema-datamodel "${prismaSchemaFile}"`); | ||
| } | ||
| if (options.fromMigrationsDirectory) { | ||
| parts.push(`--from-migrations-directory "${options.fromMigrationsDirectory}"`); | ||
| } | ||
| if (options.toMigrationsDirectory) { | ||
| parts.push(`--to-migrations-directory "${options.toMigrationsDirectory}"`); | ||
| } | ||
| if (options.fromUrl) { | ||
| parts.push(`--from-url "${options.fromUrl}"`); | ||
| } | ||
| if (options.toUrl) { | ||
| parts.push(`--to-url "${options.toUrl}"`); | ||
| } | ||
| if (options.shadowDatabaseUrl) { | ||
| parts.push(`--shadow-database-url "${options.shadowDatabaseUrl}"`); | ||
| } | ||
| if (options.script) { | ||
| parts.push('--script'); | ||
| } | ||
| if (options.exitCode) { | ||
| parts.push('--exit-code'); | ||
| } | ||
|
|
||
| // pass through any extra args | ||
| if (options.extraArgs?.length) { | ||
| parts.push(...options.extraArgs); | ||
| } | ||
|
|
||
| execPrisma(parts.join(' ')); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "migrate.ts" | grep -E "(packages/cli|src)" | head -5Repository: zenstackhq/zenstack
Length of output: 100
🏁 Script executed:
find . -type f -name "exec-utils.ts" | head -5Repository: zenstackhq/zenstack
Length of output: 101
🏁 Script executed:
# Read the migrate.ts file to see the actual code
git ls-files "packages/cli/src/actions/migrate.ts" && \
cat -n "packages/cli/src/actions/migrate.ts" | sed -n '150,220p'Repository: zenstackhq/zenstack
Length of output: 2688
🏁 Script executed:
# Read the exec-utils.ts file to understand how execPrisma works
git ls-files "packages/cli/src/utils/exec-utils.ts" && \
wc -l "packages/cli/src/utils/exec-utils.ts"Repository: zenstackhq/zenstack
Length of output: 139
🏁 Script executed:
cat -n "packages/cli/src/utils/exec-utils.ts"Repository: zenstackhq/zenstack
Length of output: 2292
Escape all arguments before shell execution.
Line 205 passes parts.join(' ') directly to execPrisma, which executes it as a shell command via execSync (exec-utils.ts:60). Values like options.fromUrl, options.toUrl, options.shadowDatabaseUrl, and options.extraArgs are not escaped, so any containing spaces, $, backticks, or quotes will be interpreted by the shell before Prisma sees them. Use proper shell escaping or switch to argv-based execution.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/actions/migrate.ts` around lines 162 - 205, runDiff
constructs a shell command by joining parts and calls execPrisma with a single
string, which allows unescaped values (options.fromUrl, options.toUrl,
options.shadowDatabaseUrl, options.extraArgs) to be interpreted by the shell;
change execPrisma usage so arguments are passed safely (either by making
execPrisma accept an argv array and call child_process.spawn/execFile without a
shell, or by properly shell-escaping each part before joining) and update
runDiff to pass the array of parts (or escaped parts) instead of parts.join('
'); reference runDiff and execPrisma (and the exec-utils helper that currently
uses execSync) when making the change.
elliots
force-pushed
the
multiple-files-fixes
branch
from
83a882d to
7c5318b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli/test/generate.test.ts (1)
287-305: Optional: extract plugin fixture writer to reduce duplication.Both new tests hand-roll similar plugin module scaffolding; a small helper in
packages/cli/test/utils.tswould keep future plugin-resolution tests shorter and easier to maintain.Also applies to: 337-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/test/generate.test.ts` around lines 287 - 305, Extract the repeated scaffolding that creates a fake node_modules plugin into a reusable helper (e.g., createPluginFixture or writePluginModule) placed in packages/cli/test/utils.ts; the helper should accept parameters for workDir, packageName, main entry (index.ts), and file contents, and it should encapsulate the mkdirSync, package.json write, and index.ts write logic currently duplicated in generate.test.ts (the block creating node_modules/my-test-plugin and its package.json/index.ts) and the similar block at 337-346; then replace the duplicated code in generate.test.ts with calls to the new helper (referencing the helper by name) to keep tests DRY and easier to maintain.
🤖 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/test/plugins/prisma-plugin.test.ts`:
- Around line 131-151: The test reveals that generateMissingOppositeRelations()
errors are being swallowed by the try-catch in generate (the block around the
generate.ts try { ... } catch { ... }), causing silent success; update the catch
in the generate() flow so it does not silently swallow exceptions from
generateMissingOppositeRelations(): either rethrow the caught error (or call
process.exit(1) after logging) so the process returns non-zero, or explicitly
detect and propagate validation/generation errors to the caller; ensure the
change references generateMissingOppositeRelations() and the try-catch in
generate so the field name collision causes a visible failure instead of a
silent missing schema file.
---
Nitpick comments:
In `@packages/cli/test/generate.test.ts`:
- Around line 287-305: Extract the repeated scaffolding that creates a fake
node_modules plugin into a reusable helper (e.g., createPluginFixture or
writePluginModule) placed in packages/cli/test/utils.ts; the helper should
accept parameters for workDir, packageName, main entry (index.ts), and file
contents, and it should encapsulate the mkdirSync, package.json write, and
index.ts write logic currently duplicated in generate.test.ts (the block
creating node_modules/my-test-plugin and its package.json/index.ts) and the
similar block at 337-346; then replace the duplicated code in generate.test.ts
with calls to the new helper (referencing the helper by name) to keep tests DRY
and easier to maintain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 730ab75b-9dae-4cec-93bc-06d31faf0494
📒 Files selected for processing (11)
packages/cli/src/actions/check.tspackages/cli/src/actions/generate.tspackages/cli/src/actions/migrate.tspackages/cli/src/index.tspackages/cli/test/generate.test.tspackages/cli/test/migrate.test.tspackages/cli/test/plugins/prisma-plugin.test.tspackages/language/res/stdlib.zmodelpackages/language/src/validators/datamodel-validator.tspackages/sdk/src/prisma/prisma-builder.tspackages/sdk/src/prisma/prisma-schema-generator.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- packages/language/src/validators/datamodel-validator.ts
- packages/cli/src/index.ts
- packages/language/res/stdlib.zmodel
- packages/cli/src/actions/generate.ts
- packages/cli/src/actions/check.ts
- packages/cli/src/actions/migrate.ts
- packages/sdk/src/prisma/prisma-builder.ts
- packages/cli/test/migrate.test.ts
Member
|
Hi @elliots , thanks for making this PR! I have a few general thoughts:
|
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
2 participants
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.
I'm importing schemas across packages, came across a few issues as I went. Happy to split it out if you prefer.
migrate diffcommand: was missing, was it meant to be?Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests