-
-
Notifications
You must be signed in to change notification settings - Fork 12
refactor(orm): simplify ORM query args #396
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Warning Rate limit exceeded@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 24 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdded a conditional SimplifyIf utility and a Simplify boolean across CRUD arg/result types, exported Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
|
Claude finished @ymc9's task —— View job PR Review: refactor(orm): simplify ORM query argsTodo List
Review Summary ✅This is a well-executed refactoring that systematically improves TypeScript type ergonomics for ORM query arguments. The changes are safe, maintain backward compatibility, and follow consistent patterns. ✅ Strengths1. Consistent API DesignAll CRUD argument types now follow a uniform pattern: // Before: Inconsistent definitions
export type CreateArgs<Schema, Model> = { data: ..., select?: ... }
export type FindManyArgs<Schema, Model> = FindArgs<Schema, Model, true>
// After: Consistent pattern with optional simplification
export type CreateArgs<Schema, Model, Simplify extends boolean = false> =
SimplifyIf<{ data: ..., select?: ... }, Simplify>2. Type Safety & Backward Compatibility
3. Performance Optimization
🔍 Technical AnalysisNo Breaking Changes
Constraint Relaxation AnalysisThe removal of
Memory & Performance Impact
🛡️ Security Assessment
🧪 Test CoverageThe refactoring maintains all existing functionality. Key areas to verify:
📋 Minor Suggestions
/**
* Conditionally simplifies a type T when Condition is true.
* Used to provide cleaner IntelliSense experience when requested.
*/
export type SimplifyIf<T, Condition extends boolean> = Condition extends true ? Simplify<T> : T;
🎯 Recommendation: APPROVE ✅This refactoring demonstrates excellent TypeScript engineering:
The conditional simplification approach is particularly clever, allowing users to opt into simplified types when needed while maintaining full compatibility for existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors the ORM query argument types to support conditional simplification through a new SimplifyIf utility type. The goal is to make type signatures cleaner and more consistent across CRUD operations while allowing callers to control whether complex intersection types are simplified.
Key changes:
- Introduced
SimplifyIf<T, Condition>utility type for conditional type simplification - Added
Simplifyboolean parameter (defaulting tofalse) to all CRUD argument types (FindArgs,CreateArgs,UpdateArgs, etc.) - Added explicit return type annotations to validator methods
- Relaxed type constraints on result types (
CountResult,AggregateResult,GroupByResult) to only require necessary structure
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/orm/src/utils/type-utils.ts | Added SimplifyIf conditional type utility for selective type simplification |
| packages/orm/src/client/crud/validator/index.ts | Added explicit return type annotations to all validator methods |
| packages/orm/src/client/crud-types.ts | Refactored all CRUD arg types to accept Simplify parameter; relaxed constraints on result types |
| packages/orm/src/client/contract.ts | Updated method signatures to use simplified types with Simplify=true in constraints |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Claude finished @ymc9's task —— View job PR Review: refactor(orm): simplify ORM query argsTodo List
Review Summary ✅This is a well-executed refactoring that systematically improves TypeScript type ergonomics for ORM query arguments. The changes are safe, maintain backward compatibility, and follow consistent patterns. ✅ Strengths1. Consistent API DesignAll CRUD argument types now follow a uniform pattern: // Before: Inconsistent definitions
export type CreateArgs<Schema, Model> = { data: ..., select?: ... }
export type FindManyArgs<Schema, Model> = FindArgs<Schema, Model, true>
// After: Consistent pattern with optional simplification
export type CreateArgs<Schema, Model, Simplify extends boolean = false> =
SimplifyIf<{ data: ..., select?: ... }, Simplify>2. Type Safety & Backward Compatibility
3. Performance Optimization
🔍 Technical AnalysisNo Breaking Changes
Constraint Relaxation AnalysisThe removal of
Memory & Performance Impact
🛡️ Security Assessment
🧪 Test CoverageThe refactoring maintains all existing functionality. Key areas to verify:
📋 Minor Suggestions
/**
* Conditionally simplifies a type T when Condition is true.
* Used to provide cleaner IntelliSense experience when requested.
*/
export type SimplifyIf<T, Condition extends boolean> = Condition extends true ? Simplify<T> : T;
🎯 Recommendation: APPROVE ✅This refactoring demonstrates excellent TypeScript engineering:
The conditional simplification approach is particularly clever, allowing users to opt into simplified types when needed while maintaining full compatibility for existing code. |
|
Claude finished @ymc9's task —— View job PR Review: refactor(orm): simplify ORM query argsTodo List
Review Summary ✅This is a well-executed refactoring that systematically improves TypeScript type ergonomics for ORM query arguments. The changes are safe, maintain backward compatibility, and follow consistent patterns. ✅ Strengths1. Consistent API DesignAll CRUD argument types now follow a uniform pattern: // Before: Inconsistent definitions
export type CreateArgs<Schema, Model> = { data: ..., select?: ... }
export type FindManyArgs<Schema, Model> = FindArgs<Schema, Model, true>
// After: Consistent pattern with optional simplification
export type CreateArgs<Schema, Model, Simplify extends boolean = false> =
SimplifyIf<{ data: ..., select?: ... }, Simplify>2. Type Safety & Backward Compatibility
3. Performance Optimization
🔍 Technical AnalysisNo Breaking Changes
Constraint Relaxation AnalysisThe removal of
Memory & Performance Impact
🛡️ Security Assessment
🧪 Test CoverageThe refactoring maintains all existing functionality. Key areas to verify:
📋 Minor Suggestions
/**
* Conditionally simplifies a type T when Condition is true.
* Used to provide cleaner IntelliSense experience when requested.
*/
export type SimplifyIf<T, Condition extends boolean> = Condition extends true ? Simplify<T> : T;
🎯 Recommendation: APPROVE ✅This refactoring demonstrates excellent TypeScript engineering:
The conditional simplification approach is particularly clever, allowing users to opt into simplified types when needed while maintaining full compatibility for existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/clients/tanstack-query/src/react.ts(3 hunks)packages/clients/tanstack-query/src/svelte.ts(3 hunks)packages/clients/tanstack-query/src/vue.ts(3 hunks)packages/clients/tanstack-query/test/react-query.test.tsx(1 hunks)packages/orm/src/client/contract.ts(14 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/clients/tanstack-query/test/react-query.test.tsx (1)
packages/clients/tanstack-query/src/react.ts (1)
Provider(87-87)
packages/orm/src/client/contract.ts (1)
packages/orm/src/client/crud-types.ts (18)
FindManyArgs(667-671)SelectSubset(510-516)SimplifiedModelResult(182-188)FindUniqueArgs(679-688)FindFirstArgs(673-677)CreateArgs(694-706)CreateManyArgs(708-712)CreateManyAndReturnArgs(714-724)UpdateArgs(850-863)UpdateManyArgs(865-869)UpdateManyAndReturnArgs(871-881)Subset(506-508)UpsertArgs(889-903)DeleteArgs(1015-1027)DeleteManyArgs(1029-1039)CountArgs(1045-1054)AggregateArgs(1074-1095)GroupByArgs(1166-1190)
packages/clients/tanstack-query/src/svelte.ts (2)
packages/clients/tanstack-query/src/react.ts (7)
ModelMutationResult(124-124)ModelMutationOptions(121-122)ModelQueryOptions(94-94)ModelQueryResult(96-96)ModelInfiniteQueryOptions(105-108)ModelInfiniteQueryResult(110-110)ModelMutationModelResult(126-138)packages/orm/src/client/crud-types.ts (6)
SimplifiedModelResult(182-188)FindUniqueArgs(679-688)SelectSubset(510-516)FindFirstArgs(673-677)FindManyArgs(667-671)Subset(506-508)
packages/clients/tanstack-query/src/vue.ts (2)
packages/clients/tanstack-query/src/react.ts (7)
ModelMutationResult(124-124)ModelMutationOptions(121-122)ModelQueryOptions(94-94)ModelQueryResult(96-96)ModelInfiniteQueryOptions(105-108)ModelInfiniteQueryResult(110-110)ModelMutationModelResult(126-138)packages/orm/src/client/crud-types.ts (16)
SimplifiedModelResult(182-188)FindUniqueArgs(679-688)SelectSubset(510-516)FindFirstArgs(673-677)FindManyArgs(667-671)CreateArgs(694-706)CreateManyAndReturnArgs(714-724)UpdateArgs(850-863)UpdateManyAndReturnArgs(871-881)UpsertArgs(889-903)DeleteArgs(1015-1027)DeleteManyArgs(1029-1039)CountArgs(1045-1054)Subset(506-508)AggregateArgs(1074-1095)GroupByArgs(1166-1190)
packages/clients/tanstack-query/src/react.ts (3)
packages/clients/tanstack-query/src/svelte.ts (3)
ModelMutationResult(112-112)ModelMutationOptions(109-110)ModelMutationModelResult(114-128)packages/clients/tanstack-query/src/vue.ts (3)
ModelMutationResult(101-101)ModelMutationOptions(97-99)ModelMutationModelResult(103-115)packages/orm/src/client/crud-types.ts (9)
SimplifiedModelResult(182-188)FindUniqueArgs(679-688)SelectSubset(510-516)FindFirstArgs(673-677)FindManyArgs(667-671)CreateArgs(694-706)CreateManyArgs(708-712)CreateManyAndReturnArgs(714-724)Subset(506-508)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (1)
packages/clients/tanstack-query/test/react-query.test.tsx (1)
28-28: LGTM! Clean removal of logging flag from test configuration.The removal of the
loggingflag simplifies the test setup while preserving the endpoint configuration. This reduces noise in test output without affecting test behavior.
|
Claude finished @ymc9's task —— View job PR Review: refactor(orm): simplify ORM query argsTodo List
Review Summary ✅This is a well-executed refactoring that systematically improves TypeScript type ergonomics for ORM query arguments. The changes are safe, maintain backward compatibility, and follow consistent patterns. ✅ Strengths1. Consistent API DesignAll CRUD argument types now follow a uniform pattern: // Before: Inconsistent definitions
export type CreateArgs<Schema, Model> = { data: ..., select?: ... }
export type FindManyArgs<Schema, Model> = FindArgs<Schema, Model, true>
// After: Consistent pattern with optional simplification
export type CreateArgs<Schema, Model, Simplify extends boolean = false> =
SimplifyIf<{ data: ..., select?: ... }, Simplify>2. Type Safety & Backward Compatibility
3. Performance Optimization
🔍 Technical AnalysisNo Breaking Changes
Constraint Relaxation AnalysisThe removal of
Memory & Performance Impact
🛡️ Security Assessment
🧪 Test CoverageThe refactoring maintains all existing functionality. Key areas to verify:
📋 Minor Suggestions
/**
* Conditionally simplifies a type T when Condition is true.
* Used to provide cleaner IntelliSense experience when requested.
*/
export type SimplifyIf<T, Condition extends boolean> = Condition extends true ? Simplify<T> : T;
🎯 Recommendation: APPROVE ✅This refactoring demonstrates excellent TypeScript engineering:
The conditional simplification approach is particularly clever, allowing users to opt into simplified types when needed while maintaining full compatibility for existing code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/clients/tanstack-query/src/react.ts(3 hunks)packages/clients/tanstack-query/src/svelte.ts(3 hunks)packages/clients/tanstack-query/src/vue.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/clients/tanstack-query/src/vue.ts (2)
packages/clients/tanstack-query/src/react.ts (3)
ModelMutationResult(124-124)ModelMutationOptions(121-122)ModelMutationModelResult(126-136)packages/orm/src/client/crud-types.ts (17)
SimplifiedModelResult(182-188)FindUniqueArgs(679-688)SelectSubset(510-516)FindFirstArgs(673-677)FindManyArgs(667-671)CreateArgs(694-706)CreateManyArgs(708-712)CreateManyAndReturnArgs(714-724)UpdateArgs(850-863)UpdateManyAndReturnArgs(871-881)UpsertArgs(889-903)DeleteArgs(1015-1027)DeleteManyArgs(1029-1039)CountArgs(1045-1054)Subset(506-508)AggregateArgs(1074-1095)GroupByArgs(1166-1190)
packages/clients/tanstack-query/src/react.ts (2)
packages/clients/tanstack-query/src/svelte.ts (3)
ModelMutationResult(112-112)ModelMutationOptions(109-110)ModelMutationModelResult(114-129)packages/orm/src/client/crud-types.ts (6)
SimplifiedModelResult(182-188)FindUniqueArgs(679-688)SelectSubset(510-516)FindFirstArgs(673-677)FindManyArgs(667-671)Subset(506-508)
packages/clients/tanstack-query/src/svelte.ts (2)
packages/clients/tanstack-query/src/react.ts (5)
ModelMutationResult(124-124)ModelMutationOptions(121-122)ModelQueryOptions(94-94)ModelInfiniteQueryOptions(105-108)ModelMutationModelResult(126-136)packages/orm/src/client/crud-types.ts (18)
SimplifiedModelResult(182-188)FindUniqueArgs(679-688)SelectSubset(510-516)FindFirstArgs(673-677)FindManyArgs(667-671)CreateArgs(694-706)CreateManyArgs(708-712)CreateManyAndReturnArgs(714-724)UpdateArgs(850-863)UpdateManyArgs(865-869)UpdateManyAndReturnArgs(871-881)UpsertArgs(889-903)DeleteArgs(1015-1027)DeleteManyArgs(1029-1039)CountArgs(1045-1054)Subset(506-508)AggregateArgs(1074-1095)GroupByArgs(1166-1190)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, sqlite)
- GitHub Check: build-test (20.x, postgresql)
🔇 Additional comments (6)
packages/clients/tanstack-query/src/svelte.ts (2)
114-129: Past review concern has been addressed.The
Arraygeneric parameter is now correctly threaded throughSimplifiedModelResultat lines 121, 126, and 127. This ensures that array-returning mutations likeuseCreateManyAndReturnanduseUpdateManyAndReturnwill correctly type their results as arrays when theArrayparameter istrue.
141-211: Hook signatures correctly updated to use simplified types.All hooks consistently use:
- True-variant argument types (e.g.,
FindFirstArgs<Schema, Model, true>)SimplifiedModelResultfor return types- Appropriate nullable types for find operations that may return null
The type-level refactor maintains consistency across all operations.
packages/clients/tanstack-query/src/react.ts (2)
126-136: Past review concern has been addressed.The
Arraygeneric is correctly propagated throughSimplifiedModelResultat lines 131, 134, and 135, ensuring array-returning mutations are properly typed.
148-253: Hook signatures correctly updated to use simplified types.All hooks consistently adopt:
- True-variant argument constraints (with the exception noted above)
SimplifiedModelResultreturn types- Appropriate nullability for operations that may not find records
The refactor successfully unifies the type surface across both regular and suspense variants.
packages/clients/tanstack-query/src/vue.ts (2)
103-113: Past review concern has been addressed.The
Arraygeneric parameter is correctly threaded throughSimplifiedModelResultat lines 108, 111, and 112, ensuring accurate typing for array-returning mutations.
125-195: Hook signatures correctly updated to use simplified types.All hooks consistently use:
- True-variant argument types (e.g.,
FindUniqueArgs<Schema, Model, true>)SimplifiedModelResultfor return types- Proper nullability for operations that may return null
The refactor successfully aligns the Vue adapter with the new type surface.
|
Claude finished @ymc9's task —— View job PR Review: refactor(orm): simplify ORM query argsTodo List
Review Summary ✅This is a well-executed refactoring that systematically improves TypeScript type ergonomics for ORM query arguments. The changes are safe, maintain backward compatibility, and follow consistent patterns. ✅ Strengths1. Consistent API DesignAll CRUD argument types now follow a uniform pattern: // Before: Inconsistent definitions
export type CreateArgs<Schema, Model> = { data: ..., select?: ... }
export type FindManyArgs<Schema, Model> = FindArgs<Schema, Model, true>
// After: Consistent pattern with optional simplification
export type CreateArgs<Schema, Model, Simplify extends boolean = false> =
SimplifyIf<{ data: ..., select?: ... }, Simplify>2. Type Safety & Backward Compatibility
3. Performance Optimization
🔍 Technical AnalysisNo Breaking Changes
Constraint Relaxation AnalysisThe removal of
Memory & Performance Impact
🛡️ Security Assessment
|
Summary by CodeRabbit
Refactor
New Features