Conversation
…#2518) 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.5.2)I've completed a comprehensive review of this release PR. Here's my analysis: Todo List:
Code Quality Assessment🟢 Excellent Aspects1. ORM Lateral Join Improvements (
2. OpenAPI nestedRoutes Feature (
3. Language Document Loading (
🟡 Minor Concerns1. Performance Considerations
2. Code Complexity
🟢 Security AssessmentNo security concerns identified:
🟢 Test CoverageComprehensive test coverage across all features:
🟢 Best PracticesCode follows excellent practices:
Specific Technical FeedbackORM Lateral Join Implementation (
|
Contributor
📝 WalkthroughWalkthroughPatch version bump from 3.5.1 to 3.5.2 across all packages, with functional enhancements for relation ordering in ORM dialects, optional partial model returns in language document loading, and nested REST API routes in OpenAPI spec generation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 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)
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 (4)
packages/language/src/document.ts (1)
120-120: Optional cleanup: deduplicate partial-model construction.The same ternary/cast is duplicated in both error branches. Consider computing it once and reusing it for cleaner maintenance.
♻️ Proposed refactor
+ const partialModel = returnPartialModelForError ? (document.parseResult.value as Model) : undefined; + if (errors.length > 0) { return { success: false, - model: returnPartialModelForError ? (document.parseResult.value as Model) : undefined, + model: partialModel, errors, warnings, }; } @@ if (additionalErrors.length > 0) { return { success: false, - model: returnPartialModelForError ? (document.parseResult.value as Model) : undefined, + model: partialModel, errors: additionalErrors, warnings, }; }Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/language/src/document.ts` at line 120, The duplicated ternary/cast using returnPartialModelForError ? (document.parseResult.value as Model) : undefined should be computed once and reused; introduce a local const (e.g. partialModel) assigned to returnPartialModelForError ? (document.parseResult.value as Model) : undefined before the error branches, then replace both occurrences at the lines referencing model with that partialModel to avoid duplication (also apply the same change for the other occurrence around line 144).packages/server/src/api/rest/openapi.ts (1)
435-435: Consider extractingchildIdParamto shared parameters.For consistency with how
idis defined ingenerateSharedParams, consider addingchildIdthere as well. This would centralize parameter definitions and make future maintenance easier.♻️ Suggested refactor
In
generateSharedParams():private generateSharedParams(): Record<string, ParameterObject> { return { id: { name: 'id', in: 'path', required: true, schema: { type: 'string' }, description: 'Resource ID', }, + childId: { + name: 'childId', + in: 'path', + required: true, + schema: { type: 'string' }, + description: 'Nested resource ID', + }, include: {Then in
buildNestedSinglePath:-const childIdParam = { name: 'childId', in: 'path', required: true, schema: { type: 'string' } }; +const childIdParam = { $ref: '#/components/parameters/childId' };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/src/api/rest/openapi.ts` at line 435, Extract the inline childIdParam into the shared params generator by adding a new entry for "childId" in generateSharedParams() (mirroring the existing "id" definition) and then replace the inline const childIdParam usage in buildNestedSinglePath with a reference to the shared param (e.g., sharedParams.childId or the appropriate lookup). Update any imports/returns of generateSharedParams so callers can access the new childId definition and ensure the schema matches { type: 'string', in: 'path', required: true } to preserve behavior.packages/server/test/openapi/rest-openapi.test.ts (1)
622-632: Consider renaming localschemavariables to avoid shadowing.The local variable
schemaon lines 624 and 630 shadows the outerschemaconstant (line 19). While functionally correct, using descriptive names likeresponseSchemaorrequestBodySchemawould improve readability.♻️ Suggested variable naming
it('nested single path GET returns single resource response', () => { const getOp = spec.paths['/user/{id}/posts/{childId}'].get; - const schema = getOp.responses['200'].content['application/vnd.api+json'].schema; - expect(schema.$ref).toBe('#/components/schemas/PostResponse'); + const responseSchema = getOp.responses['200'].content['application/vnd.api+json'].schema; + expect(responseSchema.$ref).toBe('#/components/schemas/PostResponse'); }); it('nested single path PATCH uses UpdateRequest body', () => { const patchOp = spec.paths['/user/{id}/posts/{childId}'].patch; - const schema = patchOp.requestBody.content['application/vnd.api+json'].schema; - expect(schema.$ref).toBe('#/components/schemas/PostUpdateRequest'); + const requestBodySchema = patchOp.requestBody.content['application/vnd.api+json'].schema; + expect(requestBodySchema.$ref).toBe('#/components/schemas/PostUpdateRequest'); });Apply similar changes to lines 651, 657, and 672.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/server/test/openapi/rest-openapi.test.ts` around lines 622 - 632, The tests declare a top-level const named schema which is being shadowed by local variables named schema in tests like the GET check (use getOp.responses['200'].content['application/vnd.api+json'].schema) and the PATCH check (use patchOp.requestBody.content['application/vnd.api+json'].schema); rename the local variables to descriptive names (e.g., responseSchema for the GET test and requestBodySchema for the PATCH test) to avoid shadowing and improve readability, and apply the same renaming pattern to the other occurrences mentioned (the tests around the tokens you noted at lines 651, 657, and 672).packages/orm/src/client/crud/dialects/postgresql.ts (1)
285-287: Avoidsql.raw()for the enum keywords.These values already come from a closed set, so prefer static
sqlfragments here instead of uppercasing runtime strings into raw SQL.♻️ Suggested refactor
- const dir = sql.raw(sort.toUpperCase()); - const nullsSql = nulls ? sql` NULLS ${sql.raw(nulls.toUpperCase())}` : sql``; + const dir = sort === 'asc' ? sql`ASC` : sql`DESC`; + const nullsSql = + nulls === 'first' ? sql` NULLS FIRST` : nulls === 'last' ? sql` NULLS LAST` : sql``; return sql`${expr} ${dir}${nullsSql}`;As per coding guidelines "Use Kysely query builder as escape hatch instead of raw SQL in ORM operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/orm/src/client/crud/dialects/postgresql.ts` around lines 285 - 287, The code uses sql.raw(sort.toUpperCase()) and sql.raw(nulls.toUpperCase()) which embeds runtime strings; instead map the closed sets to static sql fragments to avoid raw SQL: replace sql.raw(sort.toUpperCase()) with a conditional that returns sql`ASC` or sql`DESC` (based on the sort value) and replace sql.raw(nulls.toUpperCase()) with sql`FIRST` or sql`LAST` (based on nulls), then build dir and nullsSql using those static sql fragments when creating the final sql`${expr} ${dir}${nullsSql}`; update the code paths that set dir and nullsSql (the variables named dir and nullsSql) to use explicit mappings rather than sql.raw.
🤖 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/orm/src/client/crud/dialects/lateral-join-dialect-base.ts`:
- Around line 216-225: When handling negative payload.take in
lateral-join-dialect-base.ts, you negate the sort via this.negateSort(sort,
true) but you do not negate the nulls ordering, which breaks backward pagination
for nullable keys; update the block that prepares items to compute a separate
nulls variable (e.g., let nulls = value === 'string' ? undefined : value.nulls)
and when payload.take < 0 apply the same inversion to nulls (call an existing
helper like this.negateNulls(nulls) or implement equivalent logic) so you push
items with { expr, sort, nulls } where both sort and nulls are inverted together
for negative takes.
---
Nitpick comments:
In `@packages/language/src/document.ts`:
- Line 120: The duplicated ternary/cast using returnPartialModelForError ?
(document.parseResult.value as Model) : undefined should be computed once and
reused; introduce a local const (e.g. partialModel) assigned to
returnPartialModelForError ? (document.parseResult.value as Model) : undefined
before the error branches, then replace both occurrences at the lines
referencing model with that partialModel to avoid duplication (also apply the
same change for the other occurrence around line 144).
In `@packages/orm/src/client/crud/dialects/postgresql.ts`:
- Around line 285-287: The code uses sql.raw(sort.toUpperCase()) and
sql.raw(nulls.toUpperCase()) which embeds runtime strings; instead map the
closed sets to static sql fragments to avoid raw SQL: replace
sql.raw(sort.toUpperCase()) with a conditional that returns sql`ASC` or
sql`DESC` (based on the sort value) and replace sql.raw(nulls.toUpperCase())
with sql`FIRST` or sql`LAST` (based on nulls), then build dir and nullsSql using
those static sql fragments when creating the final sql`${expr}
${dir}${nullsSql}`; update the code paths that set dir and nullsSql (the
variables named dir and nullsSql) to use explicit mappings rather than sql.raw.
In `@packages/server/src/api/rest/openapi.ts`:
- Line 435: Extract the inline childIdParam into the shared params generator by
adding a new entry for "childId" in generateSharedParams() (mirroring the
existing "id" definition) and then replace the inline const childIdParam usage
in buildNestedSinglePath with a reference to the shared param (e.g.,
sharedParams.childId or the appropriate lookup). Update any imports/returns of
generateSharedParams so callers can access the new childId definition and ensure
the schema matches { type: 'string', in: 'path', required: true } to preserve
behavior.
In `@packages/server/test/openapi/rest-openapi.test.ts`:
- Around line 622-632: The tests declare a top-level const named schema which is
being shadowed by local variables named schema in tests like the GET check (use
getOp.responses['200'].content['application/vnd.api+json'].schema) and the PATCH
check (use patchOp.requestBody.content['application/vnd.api+json'].schema);
rename the local variables to descriptive names (e.g., responseSchema for the
GET test and requestBodySchema for the PATCH test) to avoid shadowing and
improve readability, and apply the same renaming pattern to the other
occurrences mentioned (the tests around the tokens you noted at lines 651, 657,
and 672).
🪄 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: 1075e42b-fa5e-4f29-b6d8-8bb557b2b2b8
📒 Files selected for processing (30)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/clients/client-helpers/package.jsonpackages/clients/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/language/package.jsonpackages/language/src/document.tspackages/orm/package.jsonpackages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/mysql.tspackages/orm/src/client/crud/dialects/postgresql.tspackages/plugins/policy/package.jsonpackages/schema/package.jsonpackages/sdk/package.jsonpackages/server/package.jsonpackages/server/src/api/rest/openapi.tspackages/server/test/openapi/rest-openapi.test.tspackages/testtools/package.jsonpackages/zod/package.jsonsamples/orm/package.jsontests/e2e/orm/client-api/relation/order-by-nested-includes.test.tstests/e2e/package.jsontests/regression/package.jsontests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
Comment on lines
+216
to
+225
| let sort = typeof value === 'string' ? value : value.sort; | ||
| if (payload.take !== undefined && payload.take < 0) { | ||
| // negative `take` requires negated sorting, and the result order | ||
| // will be corrected during post-read processing | ||
| sort = this.negateSort(sort, true); | ||
| } | ||
| if (typeof value === 'string') { | ||
| items.push({ expr, sort }); | ||
| } else { | ||
| items.push({ expr, sort, nulls: value.nulls }); |
Contributor
There was a problem hiding this comment.
Reverse nulls together with sort for negative take.
Backward pagination only works if the full ordering is inverted. Keeping nulls unchanged fetches the wrong window for nullable sort keys.
🐛 Proposed fix
- let sort = typeof value === 'string' ? value : value.sort;
+ let sort = typeof value === 'string' ? value : value.sort;
+ let nulls = typeof value === 'string' ? undefined : value.nulls;
if (payload.take !== undefined && payload.take < 0) {
// negative `take` requires negated sorting, and the result order
// will be corrected during post-read processing
sort = this.negateSort(sort, true);
+ if (nulls) {
+ nulls = nulls === 'first' ? 'last' : 'first';
+ }
}
if (typeof value === 'string') {
items.push({ expr, sort });
} else {
- items.push({ expr, sort, nulls: value.nulls });
+ items.push({ expr, sort, nulls });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let sort = typeof value === 'string' ? value : value.sort; | |
| if (payload.take !== undefined && payload.take < 0) { | |
| // negative `take` requires negated sorting, and the result order | |
| // will be corrected during post-read processing | |
| sort = this.negateSort(sort, true); | |
| } | |
| if (typeof value === 'string') { | |
| items.push({ expr, sort }); | |
| } else { | |
| items.push({ expr, sort, nulls: value.nulls }); | |
| let sort = typeof value === 'string' ? value : value.sort; | |
| let nulls = typeof value === 'string' ? undefined : value.nulls; | |
| if (payload.take !== undefined && payload.take < 0) { | |
| // negative `take` requires negated sorting, and the result order | |
| // will be corrected during post-read processing | |
| sort = this.negateSort(sort, true); | |
| if (nulls) { | |
| nulls = nulls === 'first' ? 'last' : 'first'; | |
| } | |
| } | |
| if (typeof value === 'string') { | |
| items.push({ expr, sort }); | |
| } else { | |
| items.push({ expr, sort, nulls }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/orm/src/client/crud/dialects/lateral-join-dialect-base.ts` around
lines 216 - 225, When handling negative payload.take in
lateral-join-dialect-base.ts, you negate the sort via this.negateSort(sort,
true) but you do not negate the nulls ordering, which breaks backward pagination
for nullable keys; update the block that prepares items to compute a separate
nulls variable (e.g., let nulls = value === 'string' ? undefined : value.nulls)
and when payload.take < 0 apply the same inversion to nulls (call an existing
helper like this.negateNulls(nulls) or implement equivalent logic) so you push
items with { expr, sort, nulls } where both sort and nulls are inverted together
for negative takes.
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
4 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
New Features
Tests
Chores