Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Nov 3, 2025

Summary by CodeRabbit

  • Refactor
    • Updated type constraints for batch update and delete operations.

Copilot AI review requested due to automatic review settings November 3, 2025 23:22
@claude
Copy link

claude bot commented Nov 3, 2025

Claude finished @ymc9's task —— View job


PR Review: Additional Typing Fixes

Todo List:

  • Read and analyze changes in packages/clients/tanstack-query/src/react.ts
  • Read and analyze changes in packages/clients/tanstack-query/test/react-query.test.tsx
  • Examine the specific changes via git diff
  • Review typing fixes for correctness and best practices
  • Check test coverage and adequacy
  • Provide comprehensive feedback on code quality, bugs, performance, and security

Review Summary

This PR contains small but important typing and test fixes for the TanStack Query integration. Overall, the changes are well-targeted and improve the code quality.

Changes Analysis

1. Type Fixes in react.ts

Lines 180 & 196: Removed incorrect array notation ([]) from generic type constraints:

  • useUpdateMany<T extends UpdateManyArgs<Schema, Model>[]>useUpdateMany<T extends UpdateManyArgs<Schema, Model>>
  • useDeleteMany<T extends DeleteManyArgs<Schema, Model>[]>useDeleteMany<T extends DeleteManyArgs<Schema, Model>>

Assessment: This is a critical fix. The previous types were incorrect because:

  • UpdateManyArgs and DeleteManyArgs are already designed to accept arrays in their data field
  • Adding [] to the constraint would make T require an array of these arg types, not the arg types themselves
  • This would have caused TypeScript compilation errors or incorrect type inference

2. Test Fix in react-query.test.tsx

Line 1537: Fixed HTTP method mock:

  • .delete(/.*/).put(/.*/)

Assessment: This is a correct bug fix. The test was mocking a DELETE request for a User update operation, but update operations use PUT. This would have caused the test to fail or behave unexpectedly.

Code Quality Assessment

Strengths

  • Targeted fixes: Changes address specific typing and testing issues without unnecessary modifications
  • Type safety: Fixes improve TypeScript type checking and developer experience
  • Test accuracy: Corrects HTTP method mocking to match actual API behavior
  • Minimal scope: Small, focused changes reduce risk of introducing new issues

Best Practices Followed

  • Maintains consistent naming conventions
  • Preserves existing API surface
  • No breaking changes to public interfaces
  • Test changes align with implementation behavior

Security & Performance

  • Security: ✅ No security concerns - changes are purely typing and test-related
  • Performance: ✅ No performance impact - compile-time fixes only

Test Coverage

The existing test suite appears comprehensive with:

  • Basic query operations (findUnique, findFirst, findMany)
  • Suspense variants
  • Infinite queries
  • Mutations (create, update, delete, upsert)
  • Optimistic updates
  • Cache invalidation scenarios
  • Nested operations

The fixed test maintains coverage for update operations with proper HTTP method validation.

Recommendations

  1. ✅ Approve: These are necessary fixes that improve type safety and test correctness
  2. Future consideration: Consider adding TypeScript compilation tests to catch similar typing issues earlier
  3. No additional tests needed: Existing test coverage is sufficient for these changes

Final Assessment

Verdict: LGTM 🚀

This PR contains essential fixes that:

  • Resolve TypeScript typing errors that would impact developer experience
  • Fix test infrastructure to properly validate API behavior
  • Maintain backward compatibility
  • Follow established patterns in the codebase

The changes are minimal, well-targeted, and improve the overall robustness of the TanStack Query integration.


@coderabbitai
Copy link

coderabbitai bot commented Nov 3, 2025

Walkthrough

Generic type constraints were simplified for useUpdateMany and useDeleteMany hooks by removing array type wrappers. A test mock HTTP method was changed from DELETE to PUT for a User update operation.

Changes

Cohort / File(s) Summary
Type signature updates
packages/clients/tanstack-query/src/react.ts
Generic type arguments simplified: useUpdateMany and useDeleteMany now constrain to single UpdateManyArgs/DeleteManyArgs instead of arrays thereof
Test mock adjustment
packages/clients/tanstack-query/test/react-query.test.tsx
HTTP mock method changed from DELETE to PUT for User update path in nested mutation flow test

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Type constraint removal from generics may affect type inference and existing hook calls—verify no downstream breakage
  • Test mock change from DELETE to PUT should align with intended API behavior; confirm this isn't a typo or unintended side effect

Poem

🐰 Hops through types with glee,
Arrays unwrapped, now wild and free,
PUT replaces DELETE's call,
Simpler signatures serve us all!
Queries dance in harmony

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): additional typing fixes' directly relates to the changeset, which updates generic type signatures for useUpdateMany and useDeleteMany hooks to fix typing issues.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/typing-tanstack

📜 Recent 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 7839d5f and 2ee738e.

📒 Files selected for processing (2)
  • packages/clients/tanstack-query/src/react.ts (2 hunks)
  • packages/clients/tanstack-query/test/react-query.test.tsx (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/clients/tanstack-query/src/react.ts (1)
packages/orm/src/client/crud-types.ts (2)
  • UpdateManyArgs (823-826)
  • DeleteManyArgs (968-971)
⏰ 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, postgresql)
  • GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (3)
packages/clients/tanstack-query/src/react.ts (2)

180-182: LGTM! Correct type constraint fix.

The generic constraint change correctly removes the array wrapper. UpdateManyArgs is already a batch operation type that handles multiple records through its structure (e.g., via the where clause), not by being an array of argument objects.


196-198: LGTM! Correct type constraint fix.

The generic constraint change correctly removes the array wrapper. DeleteManyArgs is a single object type with where and limit properties that specify which records to delete, not an array of argument objects.

packages/clients/tanstack-query/test/react-query.test.tsx (1)

1536-1538: LGTM! Correct HTTP method for User update operation.

This change fixes a test bug. The operation being tested is user.useUpdate() (line 1542), which uses the PUT method as confirmed by the implementation at line 295. The test performs a User update with nested post deletion, not a User delete operation. The previous DELETE method mock was incorrect.


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.

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 corrects type parameter definitions for batch mutation hooks and fixes a test mock configuration.

  • Fixed type parameters for useUpdateMany and useDeleteMany hooks by removing incorrect array suffix
  • Corrected test mock to use .put() instead of .delete() for a User update operation

Reviewed Changes

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

File Description
packages/clients/tanstack-query/src/react.ts Removed incorrect array suffix [] from UpdateManyArgs and DeleteManyArgs type parameters in hook definitions
packages/clients/tanstack-query/test/react-query.test.tsx Fixed nock mock to use .put() instead of .delete() for User update endpoint

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

@ymc9 ymc9 merged commit bf2ab4b into dev Nov 3, 2025
11 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