Conversation
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
Contributor
|
Claude finished @ymc9's task —— View job PR Review: merge dev to main (v3.7.1)This is a well-structured release PR merging ✅ Code Quality & Best Practices
✅ Technical Implementation ReviewNew delegateMap Feature (Primary Change)
Format Args Fix
✅ Performance Considerations
✅ Security Assessment
✅ Potential Issues AssessmentMinor Observations:
No Breaking Changes Detected:
Overall Assessment: APPROVE ✅This PR represents a high-quality release with:
The implementation follows ZenStack's architecture patterns and maintains backward compatibility while adding valuable new functionality for polymorphic model management. |
Contributor
📝 WalkthroughWalkthroughThis release introduces the ChangesDelegate Discriminator Mapping and 3.7.1 Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/cli/test/prisma-schema-gen.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/language/src/validators/datamodel-validator.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/language/test/delegate.test.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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: 1
🧹 Nitpick comments (1)
packages/sdk/src/prisma/prisma-schema-generator.ts (1)
387-391: ⚡ Quick winSimplify the filter logic for better readability.
The boolean expression on lines 389-390 is correct but difficult to parse due to implicit operator precedence, outer negation, and mixed conditions. The intent appears to be: for
ulid, remove the first argument (format string); for other ID functions, remove the second argument (format string).Consider refactoring for clarity:
♻️ Proposed refactor for clearer logic
return new PrismaFunctionCall( node.function.ref!.name, - // strip format args from id functions - node.args.filter((_, i) => ( - !(ID_FUNCTIONS.includes(node.function.ref!.name) && (node.function.ref!.name === 'ulid' && i === 0 || i === 1)) - )).map((arg) => { + // Strip format args from ID functions: + // - ulid: remove arg at index 0 (format string) + // - cuid/uuid/nanoid: remove arg at index 1 (format string, keeping version/size at index 0) + node.args.filter((_, i) => { + if (!ID_FUNCTIONS.includes(node.function.ref!.name)) { + return true; // keep all args for non-ID functions + } + const funcName = node.function.ref!.name; + return funcName === 'ulid' ? i !== 0 : i !== 1; + }).map((arg) => { const val = match(arg.value)🤖 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/sdk/src/prisma/prisma-schema-generator.ts` around lines 387 - 391, The current filter on node.args is correct but hard to read due to negation and mixed conditions; change it to an explicit, positive check: compute const name = node.function.ref!.name and then filter args to exclude the format-argument by removing index 0 when name === 'ulid' and index 1 when ID_FUNCTIONS.includes(name) for all other ID functions (i.e., keep args when not (ID_FUNCTIONS.includes(name) && ((name === 'ulid' && i === 0) || (name !== 'ulid' && i === 1)))), or better yet flip to a clear conditional that returns true to keep and false to drop using those explicit cases so node.args.filter(...) is easy to read; reference node.args, ID_FUNCTIONS and node.function.ref!.name when applying the change.
🤖 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/language/src/validators/datamodel-validator.ts`:
- Around line 529-542: The current loop in subModels.forEach uses
getDelegateMapRawValue(model) ?? model.name to derive a value but skips enum
membership validation when falling back to model.name, allowing invalid
discriminator values; update the logic in the loop (the block using seen,
getDelegateMapRawValue, and accept) to always validate the computed value
against the delegate enum members for the parent DataModel (reuse the existing
enum membership check or helper used elsewhere in datamodel-validator.ts), and
if the value is not a valid enum member emit an accept('error', ...) referencing
the model and the invalid value before checking/recording duplicates in seen.
---
Nitpick comments:
In `@packages/sdk/src/prisma/prisma-schema-generator.ts`:
- Around line 387-391: The current filter on node.args is correct but hard to
read due to negation and mixed conditions; change it to an explicit, positive
check: compute const name = node.function.ref!.name and then filter args to
exclude the format-argument by removing index 0 when name === 'ulid' and index 1
when ID_FUNCTIONS.includes(name) for all other ID functions (i.e., keep args
when not (ID_FUNCTIONS.includes(name) && ((name === 'ulid' && i === 0) || (name
!== 'ulid' && i === 1)))), or better yet flip to a clear conditional that
returns true to keep and false to drop using those explicit cases so
node.args.filter(...) is easy to read; reference node.args, ID_FUNCTIONS and
node.function.ref!.name when applying the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 276401ba-f48e-43f9-a5e9-a6d23833923b
📒 Files selected for processing (40)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/cli/test/prisma-schema-gen.test.tspackages/clients/client-helpers/package.jsonpackages/clients/fetch-client/package.jsonpackages/clients/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/tsdown-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/language/res/stdlib.zmodelpackages/language/src/validators/datamodel-validator.tspackages/language/test/delegate.test.tspackages/orm/package.jsonpackages/orm/src/client/crud-types.tspackages/orm/src/client/crud/operations/base.tspackages/orm/src/client/query-utils.tspackages/plugins/policy/package.jsonpackages/schema/package.jsonpackages/schema/src/schema.tspackages/sdk/package.jsonpackages/sdk/src/prisma/prisma-schema-generator.tspackages/sdk/src/ts-schema-generator.tspackages/server/package.jsonpackages/testtools/package.jsonpackages/zod/package.jsonsamples/orm/package.jsontests/e2e/orm/client-api/delegate.test.tstests/e2e/orm/schemas/delegate/schema.tstests/e2e/orm/schemas/delegate/schema.zmodeltests/e2e/orm/schemas/delegate/typecheck.tstests/e2e/package.jsontests/regression/package.jsontests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
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
3 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.
Summary by CodeRabbit
Release Notes
New Features
@@delegateMapattribute to map delegate sub-models to custom discriminator valuesImprovements