Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 5, 2025

Summary by CodeRabbit

  • Type System Improvements

    • Enhanced TypeScript type validation for delegate model operations to provide improved compile-time error detection and IDE support.
  • Tests

    • Added comprehensive type-checking tests for delegate model scenarios.

Copilot AI review requested due to automatic review settings November 5, 2025 20:08
@coderabbitai
Copy link

coderabbitai bot commented Nov 5, 2025

Warning

Rate limit exceeded

@ymc9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 52 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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.

📥 Commits

Reviewing files that changed from the base of the PR and between 50d19bc and 10966d4.

📒 Files selected for processing (6)
  • packages/clients/tanstack-query/src/react.ts (1 hunks)
  • packages/clients/tanstack-query/src/svelte.ts (1 hunks)
  • packages/clients/tanstack-query/src/vue.ts (1 hunks)
  • packages/clients/tanstack-query/test/react-typing-test.ts (1 hunks)
  • packages/clients/tanstack-query/test/svelte-typing-test.ts (1 hunks)
  • packages/clients/tanstack-query/test/vue-typing-test.ts (1 hunks)

Walkthrough

Introduces support for delegate models in the TanStack Query client by adding a new type that maps ineligible operations to hook names, updating the type-trimming logic, and expanding test schemas with Foo and Bar delegate models alongside corresponding type-checking tests.

Changes

Cohort / File(s) Change Summary
Type System Updates
packages/clients/tanstack-query/src/utils/types.ts
Adds new HooksOperationsIneligibleForDelegateModels type that converts operation names to hook names; updates TrimDelegateModelOperations to use the new type instead of the original operation type.
Test Schema Definitions
packages/clients/tanstack-query/test/schemas/basic/schema.zmodel, schema-lite.ts, models.ts, input.ts
Introduces Foo and Bar delegate models to the test schema (with Foo as delegate model and Bar extending Foo); exports corresponding type aliases for Foo and Bar arguments and payloads.
Framework Type-Checking Tests
packages/clients/tanstack-query/test/react-typing-test.ts, svelte-typing-test.ts, vue-typing-test.ts
Adds type-checking assertions for delegate model operations (client.foo.useCreate(), client.foo.useUpdate(), client.bar.useCreate()) marked with @ts-expect-error comments.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • The type logic change in types.ts is compact but relies on conditional type mapping; verify the template literal type construction is semantically correct.
  • Schema additions follow a consistent delegate model pattern; confirm the discriminator and inheritance setup aligns with intended delegate semantics.
  • Test assertions are repetitive across frameworks; spot-check one framework's test to ensure pattern consistency.

Possibly related PRs

  • #359: Also modifies packages/clients/tanstack-query/src/utils/types.ts with delegate model–related type changes.

Poem

🐰 A Foo and Bar did delegate with pride,
With hooks that skip what shouldn't glide,
Type tests cascade through Vue and React's dream,
This rabbit binkies at your type-safe scheme! ✨

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(tanstack): delegate hooks typing' directly relates to the main change: fixing type signatures for delegate model operations in the TanStack Query hooks layer.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Nov 5, 2025

Claude finished @ymc9's task —— View job


Code Review for TanStack Delegate Hooks Typing Fix

I'll review this PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

  • Analyze the typing changes in types.ts
  • Review test coverage and new test cases
  • Check schema changes and their impact
  • Assess code quality and best practices
  • Look for potential bugs or issues
  • Provide comprehensive feedback

📋 Review Summary

This PR successfully fixes a typing issue where delegate model hooks were not properly excluding certain operations. The fix is well-implemented and thoroughly tested.

Strengths

  1. Correct Problem Identification: The issue was that TrimDelegateModelOperations was using raw operation names (OperationsIneligibleForDelegateModels) instead of hook names (use${Capitalize<...>})

  2. Clean Implementation: The fix in packages/clients/tanstack-query/src/utils/types.ts:24-26 properly transforms operation names to hook names:

    type HooksOperationsIneligibleForDelegateModels = OperationsIneligibleForDelegateModels extends any
        ? `use${Capitalize<OperationsIneligibleForDelegateModels>}`
        : never;
  3. Comprehensive Test Coverage:

    • Added tests across all three frameworks (React, Svelte, Vue)
    • Tests correctly expect TypeScript errors for invalid operations on delegate models
    • Schema includes proper delegate model setup with @@delegate(type) and inheritance
  4. Type Safety: The fix ensures that delegate models (like Foo) cannot use creation operations (useCreate, useCreateMany, etc.) while extended models (like Bar) can use them appropriately

🔍 Technical Analysis

Core Fix (types.ts:24-32):

  • Before: Omit<T, OperationsIneligibleForDelegateModels> used raw operation names like 'create'
  • After: Omit<T, HooksOperationsIneligibleForDelegateModels> uses hook names like 'useCreate'
  • Impact: This correctly excludes hook methods from delegate model interfaces

Test Verification (*-typing-test.ts):
Lines 105-109 in each test file verify the fix works:

// @ts-expect-error delegate model
client.foo.useCreate();
client.foo.useUpdate();
client.bar.useCreate();  // This should work since Bar extends Foo

🎯 Specific Operations Excluded

Based on the ORM contract (packages/orm/src/client/contract.ts:105), delegate models exclude:

  • useCreate
  • useCreateMany
  • useCreateManyAndReturn
  • useUpsert

This makes sense architecturally - delegate models shouldn't support direct creation since they're abstract.

🔒 Security & Performance

  • Security: ✅ No security concerns - this is a compile-time typing fix
  • Performance: ✅ Zero runtime impact - purely TypeScript type manipulation
  • Memory: ✅ No additional memory overhead

📝 Code Quality

  • Consistency: ✅ Follows existing patterns in the codebase
  • Readability: ✅ Clear intent with descriptive type names
  • Maintainability: ✅ Centralized type definition that's reused across frameworks

🧪 Test Coverage Assessment

Excellent coverage:

  • ✅ Tests all three TanStack Query integrations (React, Svelte, Vue)
  • ✅ Verifies both positive and negative cases
  • ✅ Uses @ts-expect-error appropriately for expected failures
  • ✅ Tests inheritance scenario (Bar extends Foo)

🚀 Recommendations

  1. Minor: Consider adding a JSDoc comment to HooksOperationsIneligibleForDelegateModels explaining its purpose
  2. Future: This pattern could be useful for other hook transformations if needed

🎉 Final Assessment

Score: ✅ Excellent

This is a high-quality fix that:

  • Correctly addresses the typing issue
  • Maintains backward compatibility
  • Includes comprehensive test coverage
  • Follows established patterns
  • Has zero runtime impact

The PR is ready for merge. Great work on identifying and fixing this subtle but important typing issue!


Copy link
Contributor

Copilot AI left a 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 adds support for delegate models in TanStack Query hooks by excluding operations that are ineligible for delegate models from the generated hook types. The implementation maps ORM-level operation restrictions to the corresponding hook names (prefixed with "use").

  • Updated type definition to transform ORM operation names to hook names (e.g., 'create' → 'useCreate')
  • Added test coverage with delegate model schema definitions (Foo as delegate, Bar extending Foo)
  • Added typing tests to verify that ineligible hooks properly show TypeScript errors for delegate models

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/clients/tanstack-query/src/utils/types.ts Added type mapping to convert ORM operation names to hook names for delegate model exclusions
packages/clients/tanstack-query/test/schemas/basic/schema.zmodel Added Foo (delegate) and Bar (extends Foo) models to test schema
packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts Generated schema metadata for the new delegate models
packages/clients/tanstack-query/test/schemas/basic/models.ts Added type exports for Foo and Bar models
packages/clients/tanstack-query/test/schemas/basic/input.ts Generated input types for Foo and Bar models
packages/clients/tanstack-query/test/react-typing-test.ts Added TypeScript tests verifying delegate model hook restrictions
packages/clients/tanstack-query/test/vue-typing-test.ts Added TypeScript tests verifying delegate model hook restrictions
packages/clients/tanstack-query/test/svelte-typing-test.ts Added TypeScript tests verifying delegate model hook restrictions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link

@coderabbitai coderabbitai bot left a 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

♻️ Duplicate comments (2)
packages/clients/tanstack-query/test/svelte-typing-test.ts (1)

102-105: Same inconsistency as React tests.

This file has the same useUpdate inconsistency noted in the React typing test. See the comment on react-typing-test.ts lines 105-108 for details.

packages/clients/tanstack-query/test/vue-typing-test.ts (1)

103-106: Same inconsistency as other framework tests.

This file has the same useUpdate inconsistency noted in the React and Svelte typing tests. See the comment on react-typing-test.ts lines 105-108 for details.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ff17ce and 50d19bc.

📒 Files selected for processing (8)
  • packages/clients/tanstack-query/src/utils/types.ts (1 hunks)
  • packages/clients/tanstack-query/test/react-typing-test.ts (1 hunks)
  • packages/clients/tanstack-query/test/schemas/basic/input.ts (1 hunks)
  • packages/clients/tanstack-query/test/schemas/basic/models.ts (1 hunks)
  • packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts (1 hunks)
  • packages/clients/tanstack-query/test/schemas/basic/schema.zmodel (1 hunks)
  • packages/clients/tanstack-query/test/svelte-typing-test.ts (1 hunks)
  • packages/clients/tanstack-query/test/vue-typing-test.ts (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.
📚 Learning: 2025-10-21T16:09:31.218Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/executor/zenstack-query-executor.ts:63-72
Timestamp: 2025-10-21T16:09:31.218Z
Learning: In ZenStack, TypeDefs can be inherited by models. When a TypeDef contains fields with `map` attributes, those mapped field names need to be processed by the QueryNameMapper since they become part of the inheriting model's schema. Therefore, when checking if a schema has mapped names (e.g., in `schemaHasMappedNames`), both `schema.models` and `schema.typeDefs` must be inspected for `@map` and `map` attributes.

Applied to files:

  • packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts
🧬 Code graph analysis (2)
packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts (1)
packages/schema/src/expression-utils.ts (1)
  • ExpressionUtils (19-123)
packages/clients/tanstack-query/src/utils/types.ts (2)
packages/orm/src/client/contract.ts (1)
  • OperationsIneligibleForDelegateModels (798-798)
packages/schema/src/schema.ts (3)
  • SchemaDef (10-18)
  • GetModels (109-109)
  • IsDelegateModel (264-267)
⏰ 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). (4)
  • GitHub Check: Upload results
  • GitHub Check: claude-review
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (6)
packages/clients/tanstack-query/src/utils/types.ts (2)

24-26: LGTM: Clean type transformation.

The distributive conditional type correctly maps operation names to their corresponding hook names using template literals and Capitalize.


32-32: LGTM: Correct use of hook-based operation names.

The updated type correctly omits hook methods (e.g., useCreate) rather than operation names, which aligns with the actual API surface of the TanStack Query client.

packages/clients/tanstack-query/test/schemas/basic/schema.zmodel (1)

27-35: LGTM: Correct delegate model pattern.

The Foo model with @@delegate(type) and Bar extending Foo follows the proper ZenStack delegate model pattern, providing appropriate test coverage for the new typing changes.

packages/clients/tanstack-query/test/schemas/basic/models.ts (1)

13-14: LGTM: Standard generated model exports.

The generated type exports for Foo and Bar follow the established pattern and correctly reference the schema.

packages/clients/tanstack-query/test/schemas/basic/input.ts (1)

71-110: LGTM: Comprehensive generated type definitions.

The generated input types for Foo and Bar provide complete coverage of all operation arguments and payload types, following the established pattern used for other models.

packages/clients/tanstack-query/test/schemas/basic/schema-lite.ts (1)

120-167: LGTM: Well-structured delegate model schema.

The generated schema correctly represents the delegate relationship:

  • Foo is marked as isDelegate: true with subModels: ["Bar"]
  • Bar properly references baseModel: "Foo" with the discriminator field including originModel: "Foo"

This provides the necessary metadata for the type system to enforce delegate model restrictions.

@claude
Copy link

claude bot commented Nov 5, 2025

Claude finished @ymc9's task —— View job


Code Review for TanStack Delegate Hooks Typing Fix

I'll review this PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

  • Analyze the core typing changes in types.ts
  • Review test coverage and new test cases
  • Check schema changes and their impact
  • Assess code quality and best practices
  • Look for potential bugs or issues
  • Investigate the useUpdate inconsistency mentioned by CodeRabbit
  • Provide comprehensive feedback

📋 Review Summary

This PR successfully fixes a typing issue where delegate model hooks were not properly excluding certain operations. The fix is well-implemented and thoroughly tested.

Strengths

  1. Correct Problem Identification: The issue was that TrimDelegateModelOperations was using raw operation names (OperationsIneligibleForDelegateModels) instead of hook names (use${Capitalize<...>})

  2. Clean Implementation: The fix in packages/clients/tanstack-query/src/utils/types.ts:24-26 properly transforms operation names to hook names:

    type HooksOperationsIneligibleForDelegateModels = OperationsIneligibleForDelegateModels extends any
        ? `use${Capitalize<OperationsIneligibleForDelegateModels>}`
        : never;
  3. Comprehensive Test Coverage:

    • Added tests across all three frameworks (React, Svelte, Vue)
    • Tests correctly expect TypeScript errors for invalid operations on delegate models
    • Schema includes proper delegate model setup with @@delegate(type) and inheritance
  4. Type Safety: The fix ensures that delegate models (like Foo) cannot use creation operations (useCreate, useCreateMany, etc.) while extended models (like Bar) can use them appropriately

🔍 Technical Analysis

Core Fix (types.ts:24-32):

  • Before: Omit<T, OperationsIneligibleForDelegateModels> used raw operation names like 'create'
  • After: Omit<T, HooksOperationsIneligibleForDelegateModels> uses hook names like 'useCreate'
  • Impact: This correctly excludes hook methods from delegate model interfaces

Test Verification (*-typing-test.ts):
Lines 105-109 in each test file verify the fix works:

// @ts-expect-error delegate model
client.foo.useCreate();
client.foo.useUpdate();
client.bar.useCreate();  // This should work since Bar extends Foo

🎯 Specific Operations Excluded

Based on the ORM contract (packages/orm/src/client/contract.ts:798), delegate models exclude:

  • useCreate
  • useCreateMany
  • useCreateManyAndReturn
  • useUpsert

This makes sense architecturally - delegate models shouldn't support direct creation since they're abstract.

⚠️ Critical Issue Found: Test Inconsistency

Problem: The test cases have a significant inconsistency that CodeRabbit correctly identified:

// @ts-expect-error delegate model
client.foo.useCreate();    // ✅ Correctly expects error - 'create' is ineligible
client.foo.useUpdate();    // ❌ PROBLEM: No @ts-expect-error but 'update' is NOT ineligible
client.bar.useCreate();    // ❌ PROBLEM: No @ts-expect-error but Bar is delegate and 'create' IS ineligible

Root Cause: 'update' is not included in OperationsIneligibleForDelegateModels (only 'create' | 'createMany' | 'createManyAndReturn' | 'upsert'), so client.foo.useUpdate() should work fine and not produce a TypeScript error.

Fix Required: Either:

  1. Remove client.foo.useUpdate() from all test files (React, Vue, Svelte)
  2. Add @ts-expect-error to client.bar.useCreate() since Bar is a delegate model
  3. OR if update operations should be ineligible for delegates, add 'update' to OperationsIneligibleForDelegateModels

🔒 Security & Performance

  • Security: ✅ No security concerns - this is a compile-time typing fix
  • Performance: ✅ Zero runtime impact - purely TypeScript type manipulation
  • Memory: ✅ No additional memory overhead

📝 Code Quality

  • Consistency: ✅ Follows existing patterns in the codebase
  • Readability: ✅ Clear intent with descriptive type names
  • Maintainability: ✅ Centralized type definition that's reused across frameworks

🧪 Test Coverage Assessment

Excellent coverage:

  • ✅ Tests all three TanStack Query integrations (React, Svelte, Vue)
  • ✅ Verifies both positive and negative cases
  • ✅ Uses @ts-expect-error appropriately for expected failures
  • ✅ Tests inheritance scenario (Bar extends Foo)
  • BUT: Has the inconsistency issue described above

🚀 Recommendations

  1. CRITICAL: Fix the test inconsistency mentioned above
  2. Minor: Consider adding a JSDoc comment to HooksOperationsIneligibleForDelegateModels explaining its purpose
  3. Future: This pattern could be useful for other hook transformations if needed

🎉 Final Assessment

Score: ✅ Good with Required Fix

This is a high-quality fix that correctly addresses the typing issue, but requires fixing the test inconsistency before merge. The core implementation is excellent and the approach is sound.

Once the test issue is resolved, this will be ready for merge. Great work on identifying and fixing this subtle but important typing issue!


@claude
Copy link

claude bot commented Nov 5, 2025

Claude finished @ymc9's task —— View job


Code Review for TanStack Delegate Hooks Typing Fix

Review Checklist:

  • Analyze the core typing changes in types.ts
  • Review test coverage and new test cases
  • Check schema changes and their impact
  • Assess code quality and best practices
  • Look for potential bugs or issues
  • Investigate the useUpdate inconsistency mentioned by CodeRabbit
  • Provide comprehensive feedback

📋 Review Summary

This PR successfully fixes a typing issue where delegate model hooks were not properly excluding certain operations. The implementation is sound, but there are test inconsistencies that need addressing.

Strengths

  1. Correct Problem Identification: The issue was that TrimDelegateModelOperations was using raw operation names (OperationsIneligibleForDelegateModels) instead of hook names (use${Capitalize<...>})

  2. Clean Implementation: The fix in packages/clients/tanstack-query/src/utils/types.ts:24-26 properly transforms operation names to hook names:

    type HooksOperationsIneligibleForDelegateModels = OperationsIneligibleForDelegateModels extends any
        ? `use${Capitalize<OperationsIneligibleForDelegateModels>}`
        : never;
  3. Comprehensive Test Coverage: Added tests across all three frameworks (React, Svelte, Vue) with proper delegate model setup

  4. Type Safety: The fix ensures that delegate models (like Foo) cannot use creation operations (useCreate, useCreateMany, etc.) while extended models (like Bar) can use them appropriately

🔍 Technical Analysis

Core Fix (types.ts:24-32):

  • Before: Omit<T, OperationsIneligibleForDelegateModels> used raw operation names like 'create'
  • After: Omit<T, HooksOperationsIneligibleForDelegateModels> uses hook names like 'useCreate'
  • Impact: This correctly excludes hook methods from delegate model interfaces

Operations Excluded (from packages/orm/src/client/contract.ts:798):

  • useCreate
  • useCreateMany
  • useCreateManyAndReturn
  • useUpsert

This makes architectural sense - delegate models shouldn't support direct creation since they're abstract.

⚠️ Issues Found

1. Test Inconsistency in All Framework Test Files

In packages/clients/tanstack-query/test/react-typing-test.ts:105-109 (and similar in Svelte/Vue):

// @ts-expect-error delegate model
client.foo.useCreate();

client.foo.useUpdate();  // ❌ This should NOT error!
client.bar.useCreate();  // ❌ Missing @ts-expect-error annotation

Problems:

  • client.foo.useUpdate() is used without @ts-expect-error but 'update' is NOT in OperationsIneligibleForDelegateModels
  • Since update operations ARE allowed on delegate models, this line should work fine
  • client.bar.useCreate() should have @ts-expect-error since Bar is also a delegate model and create is ineligible

Recommended Fix:

// @ts-expect-error delegate model
client.foo.useCreate();

client.foo.useUpdate();  // ✅ This should work fine - no error expected

// @ts-expect-error delegate model  
client.bar.useCreate();

🔒 Security & Performance

  • Security: ✅ No security concerns - this is a compile-time typing fix
  • Performance: ✅ Zero runtime impact - purely TypeScript type manipulation
  • Memory: ✅ No additional memory overhead

📝 Code Quality Assessment

  • Consistency: ✅ Follows existing patterns in the codebase
  • Readability: ✅ Clear intent with descriptive type names
  • Maintainability: ✅ Centralized type definition that's reused across frameworks
  • Schema Design: ✅ Proper delegate model setup with @@delegate(type) and inheritance

🧪 Test Coverage Assessment

Excellent coverage overall:

  • ✅ Tests all three TanStack Query integrations (React, Svelte, Vue)
  • ✅ Tests inheritance scenario (Bar extends Foo)
  • ✅ Uses @ts-expect-error appropriately for expected failures
  • ❌ Test expectations don't match the actual type definitions (the useUpdate issue)

🎯 Recommendations

  1. Fix Test Inconsistencies: Update all three typing test files to remove the incorrect expectation around useUpdate and add proper @ts-expect-error for client.bar.useCreate()

  2. Consider Documentation: Add a JSDoc comment to HooksOperationsIneligibleForDelegateModels explaining its purpose for future maintainers

🎉 Final Assessment

Score: ✅ Approved with Minor Issues

This is a high-quality fix that correctly addresses the core typing issue. The implementation is sound and follows established patterns. The only issues are test inconsistencies that don't affect the functionality but should be corrected for accuracy.

The PR is ready for merge after fixing the test inconsistencies mentioned above.


@ymc9 ymc9 merged commit e7f191c into dev Nov 5, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants