merge dev to main (v2.7.4) #1808
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request includes several updates across multiple files. The version number in Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (4)
packages/server/src/next/app-route-handler.ts (1)
14-14: Consider enhancing version compatibility documentation.While the remark about async params is helpful, consider adding more specific version compatibility information.
- * @remarks Since Next.js 15, `context.params` is asynchronous and must be awaited. + * @remarks Since Next.js 15, `context.params` is asynchronous and must be awaited. + * For Next.js versions < 15, `context.params` is synchronously available.packages/schema/src/plugins/zod/generator.ts (1)
295-298: LGTM: Enhanced scalar fields filter to include ID fields.The logic correctly ensures ID fields are always included in the schema generation, while maintaining existing checks for regular fields and foreign keys.
Consider adding a comment explaining why ID fields are always included, for better maintainability:
scalarFields = model.fields.filter( (field) => - // id fields are always included + // ID fields are always included to ensure proper model identification isIdField(field) || // regular fields only (!isDataModel(field.type.reference?.ref) && !isForeignKeyField(field)) );packages/plugins/openapi/src/rest-generator.ts (1)
850-853: LGTM! Consider enhancing documentation.The logic for handling compound IDs is correct and follows REST API best practices by exposing all fields for read operations while controlling field visibility for write operations.
Consider adding a code example in the comment to illustrate how compound IDs are handled differently in read vs. write operations. For example:
- // For compound ids each component is also exposed as a separate fields for read operations, - // but not required for write operations + // For compound ids each component is also exposed as a separate fields for read operations, + // but not required for write operations. Example: + // For a model with compound ID (id1, id2): + // - Read: exposes both 'id1' and 'id2' as separate fields + // - Write: requires only the composite 'id' fieldpackages/server/tests/api/rest.test.ts (1)
1778-1802: Consider enhancing test coverage for PostLikeInfo creation.While the test correctly verifies the basic creation flow, consider adding:
- Assertions for the response body to verify the created entity's attributes and relationships
- Test cases for error scenarios (e.g., creating with invalid PostLike ID)
Add these assertions after the status check:
expect(r.status).toBe(201); +expect(r.body.data).toMatchObject({ + type: 'postLikeInfo', + attributes: { + text: 'LikeInfo1', + postId: 1, + userId: 'user1' + }, + relationships: { + postLike: { + data: { type: 'postLike', id: `1${idDivider}user1` } + } + } +});And add this test case for error handling:
it('fails to create postLikeInfo with invalid postLike', async () => { const r = await handler({ method: 'post', path: '/postLikeInfo', query: {}, requestBody: { data: { type: 'postLikeInfo', attributes: { text: 'LikeInfo1' }, relationships: { postLike: { data: { type: 'postLike', id: `999${idDivider}invalid` } } } } }, prisma }); expect(r.status).toBe(404); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (15)
package.jsonis excluded by!**/*.jsonpackages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/misc/redwood/package.jsonis excluded by!**/*.jsonpackages/plugins/openapi/package.jsonis excluded by!**/*.jsonpackages/plugins/openapi/tests/baseline/rest-3.0.0.baseline.yamlis excluded by!**/*.yamlpackages/plugins/openapi/tests/baseline/rest-3.1.0.baseline.yamlis excluded by!**/*.yamlpackages/plugins/swr/package.jsonis excluded by!**/*.jsonpackages/plugins/tanstack-query/package.jsonis excluded by!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by!**/*.jsonpackages/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.json
📒 Files selected for processing (8)
- packages/ide/jetbrains/build.gradle.kts (1 hunks)
- packages/plugins/openapi/src/rest-generator.ts (1 hunks)
- packages/schema/src/plugins/enhancer/enhance/index.ts (2 hunks)
- packages/schema/src/plugins/zod/generator.ts (2 hunks)
- packages/server/src/api/rest/index.ts (8 hunks)
- packages/server/src/next/app-route-handler.ts (2 hunks)
- packages/server/tests/api/rest.test.ts (2 hunks)
- tests/regression/tests/issue-1763.test.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ide/jetbrains/build.gradle.kts
🧰 Additional context used
🪛 Biome
packages/schema/src/plugins/enhancer/enhance/index.ts
[error] 546-548: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.(lint/correctness/noFlatMapIdentity)
packages/server/src/api/rest/index.ts
[error] 1255-1255: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
[error] 1277-1277: Avoid the use of spread (
...) syntax on accumulators.Spread syntax should be avoided on accumulators (like those in
.reduce) because it causes a time complexity ofO(n^2).
Consider methods such as .splice or .push instead.(lint/performance/noAccumulatingSpread)
🔇 Additional comments (9)
tests/regression/tests/issue-1763.test.ts (2)
1-4: LGTM! Test structure follows best practices.The test is well-structured using Jest's describe/it pattern and properly handles async operations.
7-16: Consider adding constraints and documentation for the Post model.A few suggestions to improve the model definition:
- Add validation or constraints for the
typefield since it's used for delegation- Consider documenting the purpose and implications of using
@@delegate(type)- Review if unrestricted access (
@@allow('all', true)) is intentional and secure for your use caseLet's check if there are any existing constraints or documentation for the Post model:
✅ Verification successful
Based on the search results, I can now provide a final response regarding the Post model's delegation and access control:
The
@@delegate(type)directive usage is valid and follows established patternsThe codebase shows consistent usage of
@@delegate(type)for implementing polymorphic behavior, where:
- The
typefield is commonly used as a discriminator field for delegation- Unrestricted access (
@@allow('all', true)) is a common pattern when using delegation, as access control is typically handled by the delegated typesThe current implementation aligns with these patterns. However, consider:
- Adding documentation comments explaining the expected
typevalues and their corresponding delegated models- If needed, add validation for the
typefield using@allowattribute to restrict it to specific values🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing Post model definitions or documentation rg -A 5 "model\s+Post\s*{" --type tsLength of output: 134
Script:
#!/bin/bash # Search for Post model definitions with fixed regex rg -A 10 "model Post" --type ts # Search for any documentation or comments about Post model and type delegation rg -B 3 -A 3 "@@delegate.*type" --type ts # Search for other models using similar delegation pattern rg "@@delegate" --type tsLength of output: 133292
packages/server/src/next/app-route-handler.ts (2)
9-9: LGTM: Type definition properly handles async params.The updated type definition correctly accommodates both Promise-based and direct object access patterns for
context.params, ensuring compatibility with Next.js 15's asynchronous params while maintaining backward compatibility.
31-49: Verify error handling consistency across route handlers.Let's ensure the error handling patterns are consistent across all route handlers using this factory.
✅ Verification successful
Let me gather more information about the error handling patterns by examining the route handler factory implementation and its usage.
Error handling patterns are consistent and well-structured
The error handling in the route handler factory follows a consistent pattern where:
- All error responses use
NextResponse.json()with appropriate status codes- Each error includes a descriptive message
- Server errors (500) are used for runtime failures
- Client errors (400) are used for invalid requests
- The handler properly delegates successful responses from the RPC handler
The changes in the reviewed code segment maintain this consistency by properly handling the asynchronous
context.paramswith appropriate error responses.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for route handler implementations using this factory echo "Searching for route handler implementations..." rg -l "app-route-handler" --type ts # Check error handling patterns in route files echo "Analyzing error handling patterns..." rg "NextResponse\.json.*status:.*[45]\d{2}" --type tsLength of output: 777
Script:
#!/bin/bash # Check the route handler factory implementation echo "Examining route handler factory implementation..." cat packages/server/src/next/app-route-handler.ts # Check how this factory is used in index.ts echo "Checking factory usage..." cat packages/server/src/next/index.ts # Look for any other NextResponse usage patterns echo "Looking for other NextResponse patterns..." rg "NextResponse" -A 2 --type tsLength of output: 6514
packages/schema/src/plugins/zod/generator.ts (2)
13-13: LGTM: Import addition for ID field handling.The addition of
isIdFieldimport is well-placed and aligns with the enhanced ID field handling functionality.
295-298: Verify the impact on existing model schemas.Let's ensure that ID fields are correctly identified and included in the generated schemas.
✅ Verification successful
Let me gather more information to verify the ID field handling.
Let me check one more thing to understand the isIdField implementation.
Let me check one final thing about the isIdField implementation.
ID fields are correctly identified and handled in schema generation
Based on the verification results:
- The
isIdFieldimplementation correctly identifies ID fields by checking for the@idattribute- The schema generator properly includes ID fields in the generated schemas through
makeFieldSchema- Multiple test models across the codebase demonstrate consistent ID field usage with both
@id()and appropriate types (String/Int)- The change to always include ID fields aligns with standard schema practices where IDs are essential model identifiers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify ID field handling in model schemas # Expected: Find model definitions with ID fields and their corresponding schema generation # Test 1: Find model definitions with ID fields echo "=== Models with ID fields ===" ast-grep --pattern 'model $MODEL { $$$ @id $FIELD $$$ }' # Test 2: Check corresponding schema generation echo -e "\n=== Generated schemas with ID fields ===" rg -A 5 'baseSchema = z.object\(' packages/schema/src/plugins/zod/Length of output: 837
Script:
#!/bin/bash # Let's check the implementation of isIdField and related functions echo "=== isIdField implementation ===" rg -A 5 "function isIdField" packages/schema/src/ # Check for ID field usage in model definitions echo -e "\n=== ID field usage in models ===" rg -B 2 -A 2 "@id" --type-add 'prisma:*.prisma' --type prisma # Check how ID fields are processed in the schema generation echo -e "\n=== ID field processing in schema generation ===" ast-grep --pattern 'isIdField($FIELD)'Length of output: 6377
Script:
#!/bin/bash # Find the isIdField implementation and its usage context echo "=== isIdField implementation and related utilities ===" rg -B 5 -A 10 "export function isIdField" --type typescript # Check the field type handling in the generator echo -e "\n=== Field type handling in generator ===" rg -B 3 -A 3 "makeFieldSchema" packages/schema/src/plugins/zod/Length of output: 4253
Script:
#!/bin/bash # Search for isIdField implementation with ts extension echo "=== isIdField implementation ===" rg -B 5 -A 10 "export function isIdField" --type ts # Check for any model field utilities echo -e "\n=== Model field utilities ===" rg -B 2 -A 2 "function isIdField|function isForeignKeyField" --type tsLength of output: 1763
packages/schema/src/plugins/enhancer/enhance/index.ts (2)
Line range hint
518-533: LGTM: Parameter rename improves type consistencyThe parameter rename from
delegateModelstodelegateInfobetter aligns with the type definition and improves code clarity.
539-541: LGTM: Removed unused parameter prefixThe removal of the underscore prefix from
_delegateInfocorrectly reflects that the parameter is now used in the implementation.packages/server/tests/api/rest.test.ts (1)
77-87: LGTM: Well-structured schema changes for PostLike and PostLikeInfo models.The schema changes properly define the one-to-many relationship between PostLike and PostLikeInfo, with appropriate fields and compound foreign key reference.
…satisfy Next15's linter (#1809)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/server/src/next/app-route-handler.ts (2)
12-14: Consider enhancing the documentation with version details.The documentation could be more specific about which Next.js version introduced this change.
- * @remarks Since Next.js 15, `context.params` is asynchronous and must be awaited. + * @remarks Since Next.js 13.5, `context.params` is asynchronous and must be awaited. + * @see https://nextjs.org/docs/app/api-reference/file-conventions/route-segment-config#dynamic-parameters
41-49: LGTM! Consider consistent error handling.The path parameter validation and processing logic is correct. For consistency, consider applying the same structured error response format here as suggested in the previous review comment.
if (!params.path) { return NextResponse.json( - { message: 'missing path parameter' }, + { + error: 'MISSING_PATH_PARAM', + message: 'The required path parameter is missing', + expected: 'Array of path segments' + }, { - status: 400, + status: 422, } ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/server/src/next/app-route-handler.ts (2 hunks)
🔇 Additional comments (2)
packages/server/src/next/app-route-handler.ts (2)
35-39: Previous review comment about error handling structure is still applicable.The error handling implementation could benefit from the structured error response format suggested in the previous review.
9-9: Verify the breaking change impact across the codebase.The change of
Context.paramsto be async is a breaking change that requires all consumers to await the params.Let's verify all usages of this type in the codebase:
✅ Verification successful
Let me search for more specific usage patterns to ensure we haven't missed any implementations.
Let me check the import in index.ts to see how this type is being used.
Breaking change is properly contained and handled
The change of
Context.paramsto be async is safely contained within theapp-route-handler.tsfile:
- The type is internal and not exported
- The async nature is properly handled in the implementation with try-catch
- The factory function's return type remains unchanged
- No other files in the codebase directly use this type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potential usage of Context type or params access rg -l "Context.*params|context\.params" --type tsLength of output: 97
Script:
#!/bin/bash # Search for actual implementations and usages with context rg "Context.*params|context\.params" -A 5 --type ts # Also search for any imports of this file to catch type usage rg -l "from ['\"].*app-route-handler" --type tsLength of output: 1937
Script:
#!/bin/bash # Check the content of index.ts to see how the type is used cat packages/server/src/next/index.tsLength of output: 1856
No description provided.