Skip to content

feat(fetch-client): implement fetch-based CRUD API client#2651

Open
ymc9 wants to merge 5 commits intodevfrom
feat/fetch-client
Open

feat(fetch-client): implement fetch-based CRUD API client#2651
ymc9 wants to merge 5 commits intodevfrom
feat/fetch-client

Conversation

@ymc9
Copy link
Copy Markdown
Member

@ymc9 ymc9 commented May 6, 2026

Summary

A new @zenstackhq/fetch-client package — a simple, fetch-based, non-reactive client that consumes the RPC-style auto CRUD API. Sits alongside @zenstackhq/tanstack-query for consumers who want plain async/await without a reactive layer.

What's in it

  • Per-model CRUD operations derived from AllModelOperations, then trimmed by slicing options via a key-filter mapped type that preserves per-call generic narrowing (so select / include still narrow result types).
  • findUniqueOrThrow / findFirstOrThrow — implemented client-side; raise a typed CrudError with code: CrudErrorCode.NotFound and the offending model name.
  • client.\$transaction([...]) — sequential transactions accepting a typed array of { model, op, args } operations. Result tuple is typed per-position via TransactionResults.
  • client.\$procs — custom procedures exposed with query / mutate based on the procedure shape.
  • Endpoint is required and validated at construction time as a fully qualified URL.
  • ExtQueryArgs / ExtResult are threaded through model ops and transactions, so plugin-extended query args / result fields are reflected in user-facing types.

Cross-package refactors

  • Promoted TransactionOperation / TransactionResults types and the CUSTOM_PROC_ROUTE_NAME / TRANSACTION_ROUTE_PREFIX constants from tanstack-query into @zenstackhq/client-helpers so both clients share them. tanstack-query re-exports unchanged at its existing import paths — no churn at call sites.
  • Moved DEFAULT_QUERY_ENDPOINT to tanstack-query (where it actually belongs).

Test plan

  • pnpm --filter @zenstackhq/fetch-client build — clean (tsc, tsdown, test:generate, test:typecheck)
  • pnpm --filter @zenstackhq/fetch-client test60 tests pass (47 runtime + 13 type-level)
  • pnpm --filter @zenstackhq/client-helpers build — clean
  • pnpm --filter @zenstackhq/tanstack-query exec tsc --noEmit -p tsconfig.general.json — clean
  • Reviewer to verify slicing / ExtQueryArgs / ExtResult typing in their own project

Test schemas use real .zmodel files generated via the existing scripts/test-generate.ts (mirroring tanstack-query's setup).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Introduced a new fetch-based client with TypeScript-typed CRUD operations, custom procedures, and sequential transactions.
    • Added CrudError handling with NotFound error code.
  • Chores

    • Reorganized internal constants and transaction types across client packages for improved modularity.

A simple fetch-based, non-reactive client library that consumes the
RPC-style auto CRUD API.

- Per-model CRUD operations (`findUnique`, `findMany`, `create`, etc.)
  derived from the ORM's `AllModelOperations`, with slicing applied via
  a key-filter mapped type that preserves per-call generic narrowing.
- `findUniqueOrThrow` / `findFirstOrThrow` raise a typed `CrudError`
  with `code: CrudErrorCode.NotFound` and the offending model name.
- Sequential transactions via `client.\$transaction([...])` accepting an
  array of typed `{ model, op, args }` operations; result tuple is
  typed per-position via `TransactionResults`.
- Custom procedures exposed under `client.\$procs` with `query` /
  `mutate` based on the procedure shape.
- Endpoint is required and validated as a fully qualified URL.
- Threads `ExtQueryArgs` and `ExtResult` through model ops and
  transactions so plugin-extended query args / result fields are
  reflected in user-facing types.

Refactors:
- Move `TransactionOperation` / `TransactionResults` types and the
  `CUSTOM_PROC_ROUTE_NAME` / `TRANSACTION_ROUTE_PREFIX` constants into
  `@zenstackhq/client-helpers` so both clients can share them.
- Move the `DEFAULT_QUERY_ENDPOINT` constant to tanstack-query (where
  it belongs).

Tests: 47 runtime tests + 13 type-level tests covering CRUD ops, HTTP
methods, SuperJSON serialization, error handling, transactions, slicing,
ExtQueryArgs, and ExtResult propagation. Test schemas use real ZModel
files generated via the existing `scripts/test-generate.ts`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Claude finished @ymc9's task —— View job


PR Review: feat(fetch-client): add @zenstackhq/fetch-client package

I've completed my review of the new @zenstackhq/fetch-client package. Here's my comprehensive feedback:

Review Tasks

  • Analyze main fetch-client implementation
  • Review cross-package refactoring changes
  • Examine test coverage and quality
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Provide overall feedback and recommendations

🟢 Overall Assessment: Strong Implementation

This is a well-designed and implemented addition to the ZenStack ecosystem. The code quality is high, follows established patterns, and provides excellent type safety.

Code Quality & Best Practices

Excellent Type Safety

  • Sophisticated use of mapped types to preserve per-call generics (src/index.ts:106-120)
  • Proper constraint propagation through ExtQueryArgs and ExtResult threading
  • Good use of conditional types for optional procedure integration (src/index.ts:94-97)

Clean Architecture

  • Well-separated concerns with distinct functions for read/write operations
  • Consistent naming conventions matching the existing codebase
  • Proper abstraction of common functionality in helper functions

Error Handling

  • Custom CrudError class with typed error codes (src/index.ts:43-55)
  • Proper validation of endpoint URLs with helpful error messages (src/index.ts:159-170)
  • Client-side implementation of *OrThrow methods with appropriate error context

Test Coverage Analysis

Comprehensive Coverage (60 tests)

  • Runtime tests: 47 tests covering all CRUD operations, transactions, procedures, error scenarios
  • Type-level tests: 13 tests ensuring TypeScript inference works correctly
  • Good test organization with separate schemas for different scenarios
  • Proper mocking of fetch requests with realistic response structures

Test Quality

  • Tests verify both HTTP method/URL correctness and response handling
  • Good edge case coverage (empty endpoints, trailing slashes, custom fetch functions)
  • Type-level tests ensure slicing and result narrowing work as expected

Security Considerations

Good Security Practices

  • Endpoint validation prevents relative URL injection (src/index.ts:164-166)
  • Proper JSON marshaling using established utilities
  • No hardcoded credentials or sensitive data
  • Uses existing security patterns from the ecosystem

⚠️ Minor Security Notes

  • Consider adding request timeout configuration for production use
  • The endpoint validation could potentially be bypassed with javascript: or data: URLs, though these would likely fail in practice

Performance Considerations

Efficient Design

  • Lazy construction of model operations in buildModelOperations
  • Reuses established fetch utilities and marshaling logic
  • Sequential transactions are appropriately implemented server-side
  • No unnecessary object creation or complex computations

💡 Performance Opportunities

  • Consider connection pooling options for high-throughput scenarios (could be added to fetch options)
  • The model operations could potentially be memoized if endpoint/fetch config is stable

Cross-Package Refactoring

Well-Executed Refactoring

  • Smart promotion of shared types to @zenstackhq/client-helpers
  • Maintains backward compatibility in tanstack-query through re-exports
  • Clean separation of concerns (moved DEFAULT_QUERY_ENDPOINT to where it belongs)
  • No breaking changes at call sites

Potential Issues Found

🔍 Minor Issues

  1. Type Assertion in Schema Access (src/index.ts:279):

    const procedures = (schema as any).procedures

    Consider using a more type-safe approach or adding a comment explaining why this is necessary.

  2. Missing Input Validation in Procedures (src/index.ts:285-292):
    Custom procedures don't validate input types - though this may be intentional to match server-side validation patterns.

Specific Code Comments

Well-Implemented Features

  1. Transaction Type Safety (client-helpers/src/transaction.ts:56-69):
    The TransactionResults type correctly maps each operation to its precise return type, preserving per-position typing.

  2. HTTP Method Selection (src/index.ts:172-193):
    Clean separation between GET (read) and POST/PUT/DELETE (write) operations with appropriate body handling.

  3. Endpoint Normalization (src/index.ts:159-170):
    Robust endpoint validation with clear error messages and proper URL construction.

Recommendations

🔧 Minor Improvements

  1. Add Request Timeout Option:

    export type FetchClientOptions = {
      endpoint: string;
      fetch?: FetchFn;
      timeout?: number; // Add this
    };
  2. Consider Adding Retry Logic Configuration:
    For production resilience, consider exposing retry options.

  3. Enhance Type Safety for Procedures:

    // Instead of (schema as any).procedures
    const procedures = 'procedures' in schema ? schema.procedures : undefined;

Final Verdict

✅ APPROVED - This is a high-quality implementation that:

  • Follows established patterns and conventions
  • Provides excellent type safety and developer experience
  • Has comprehensive test coverage
  • Maintains backward compatibility
  • Adds valuable functionality to the ZenStack ecosystem

The refactoring work is also well-executed and improves code organization without breaking existing functionality.


@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 1fc7ed2e-23bf-419a-8cd8-8b98d089d1c0

📥 Commits

Reviewing files that changed from the base of the PR and between 6811e77 and 34c00f7.

📒 Files selected for processing (1)
  • packages/clients/fetch-client/package.json
✅ Files skipped from review due to trivial changes (1)
  • packages/clients/fetch-client/package.json

📝 Walkthrough

Walkthrough

A new fetch-based client package is introduced alongside a refactoring that extracts shared transaction types and route constants into a centralized client-helpers module. Existing consumer packages (tanstack-query) are updated to import from the new shared locations.

Changes

Centralized Client Infrastructure & Fetch Client

Layer / File(s) Summary
Shared Type Infrastructure
packages/clients/client-helpers/src/constants.ts, packages/clients/client-helpers/src/transaction.ts, packages/clients/client-helpers/src/index.ts
New transaction types (TransactionOperation, TransactionResults) and route constants (CUSTOM_PROC_ROUTE_NAME, TRANSACTION_ROUTE_PREFIX) extracted to client-helpers; DEFAULT_QUERY_ENDPOINT removed from this layer and pushed to consumers.
Existing Package Wiring
packages/clients/tanstack-query/src/common/constants.ts, packages/clients/tanstack-query/src/common/transaction.ts, packages/clients/tanstack-query/src/common/types.ts
tanstack-query re-exports transaction types from client-helpers; CUSTOM_PROC_ROUTE_NAME and TRANSACTION_ROUTE_PREFIX now imported from client-helpers; DEFAULT_QUERY_ENDPOINT defined locally.
Framework Integration Updates
packages/clients/tanstack-query/src/react.ts, packages/clients/tanstack-query/src/svelte/index.svelte.ts, packages/clients/tanstack-query/src/vue.ts
Framework-specific files updated to import DEFAULT_QUERY_ENDPOINT from consolidated common/constants location.
Fetch Client Core
packages/clients/fetch-client/src/index.ts
New typed fetch client implementation (createClient, ModelOperations, FetchClient) with CrudError handling, model operation surfaces, procedure support, and sequential $transaction API.
Fetch Client Package Metadata & Infrastructure
packages/clients/fetch-client/package.json, packages/clients/fetch-client/tsconfig.json, packages/clients/fetch-client/tsconfig.test.json, packages/clients/fetch-client/tsdown.config.ts, packages/clients/fetch-client/vitest.config.ts, packages/clients/fetch-client/eslint.config.js
Build, test, and linting infrastructure established for the new package with TypeScript, Vitest, and ESLint configuration.
Test Schemas & Type Coverage
packages/clients/fetch-client/test/schemas/basic/schema.zmodel, packages/clients/fetch-client/test/schemas/basic/schema-lite.ts, packages/clients/fetch-client/test/schemas/no-procs/schema.zmodel, packages/clients/fetch-client/test/schemas/no-procs/schema-lite.ts
Test schemas defined for both procedure-enabled and procedure-free scenarios with auto-generated TypeScript schema definitions.
Fetch Client Tests & Type Validation
packages/clients/fetch-client/test/fetch-client.test.ts, packages/clients/fetch-client/test/typing.test-d.ts
Comprehensive test coverage for read/write operations, error handling, serialization, procedures, and transactions; TypeScript type-level tests validate result narrowing, slicing, ExtQueryArgs, and ExtResult constraints.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐰 A fetch client hops into sight,
Types unified, shining so bright,
Constants consolidated with care,
Transactions flow through the air,
New models dance—what a delight!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat(fetch-client): implement fetch-based CRUD API client' directly and clearly summarizes the main change—introducing a new fetch-based client implementation for CRUD API operations, which aligns with the primary objective of implementing the @zenstackhq/fetch-client package.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/fetch-client

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.

@ymc9 ymc9 changed the title feat(fetch-client): add @zenstackhq/fetch-client package feat(fetch-client): implement fetch-based CRUD API client May 6, 2026
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Claude finished @ymc9's task —— View job


PR Review: feat(fetch-client): implement fetch-based CRUD API client

I've completed my review of the new @zenstackhq/fetch-client package. Here's my comprehensive feedback:

Review Tasks

  • Analyze main fetch-client implementation
  • Review cross-package refactoring changes
  • Examine test coverage and quality
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Provide overall feedback and recommendations

🟢 Overall Assessment: Strong Implementation

This is a well-designed and implemented addition to the ZenStack ecosystem. The code quality is high, follows established patterns, and provides excellent type safety.

Code Quality & Best Practices

Excellent Type Safety

  • Sophisticated use of mapped types to preserve per-call generics (src/index.ts:106-120)
  • Proper constraint propagation through ExtQueryArgs and ExtResult threading
  • Good use of conditional types for optional procedure integration (src/index.ts:94-97)

Clean Architecture

  • Well-separated concerns with distinct functions for read/write operations
  • Consistent naming conventions matching the existing codebase
  • Proper abstraction of common functionality in helper functions

Error Handling

  • Custom CrudError class with typed error codes (src/index.ts:43-55)
  • Proper validation of endpoint URLs with helpful error messages (src/index.ts:159-170)
  • Client-side implementation of *OrThrow methods with appropriate error context

Test Coverage Analysis

Comprehensive Coverage (60 tests)

  • Runtime tests: 47 tests covering all CRUD operations, transactions, procedures, error scenarios
  • Type-level tests: 13 tests ensuring TypeScript inference works correctly
  • Good test organization with separate schemas for different scenarios
  • Proper mocking of fetch requests with realistic response structures

Test Quality

  • Tests verify both HTTP method/URL correctness and response handling
  • Good edge case coverage (empty endpoints, trailing slashes, custom fetch functions)
  • Type-level tests ensure slicing and result narrowing work as expected

Security Considerations

Good Security Practices

  • Endpoint validation prevents relative URL injection (src/index.ts:164-166)
  • Proper JSON marshaling using established utilities
  • No hardcoded credentials or sensitive data
  • Uses existing security patterns from the ecosystem

⚠️ Minor Security Notes

  • Consider adding request timeout configuration for production use
  • The endpoint validation could potentially be bypassed with javascript: or data: URLs, though these would likely fail in practice

Performance Considerations

Efficient Design

  • Lazy construction of model operations in buildModelOperations
  • Reuses established fetch utilities and marshaling logic
  • Sequential transactions are appropriately implemented server-side
  • No unnecessary object creation or complex computations

💡 Performance Opportunities

  • Consider connection pooling options for high-throughput scenarios (could be added to fetch options)
  • The model operations could potentially be memoized if endpoint/fetch config is stable

Cross-Package Refactoring

Well-Executed Refactoring

  • Smart promotion of shared types to @zenstackhq/client-helpers
  • Maintains backward compatibility in tanstack-query through re-exports
  • Clean separation of concerns (moved DEFAULT_QUERY_ENDPOINT to where it belongs)
  • No breaking changes at call sites

Potential Issues Found

🔍 Minor Issues

  1. Type Assertion in Schema Access (src/index.ts:279):

    const procedures = (schema as any).procedures

    Consider using a more type-safe approach or adding a comment explaining why this is necessary.

  2. Missing Input Validation in Procedures (src/index.ts:285-292):
    Custom procedures don't validate input types - though this may be intentional to match server-side validation patterns.

Specific Code Comments

Well-Implemented Features

  1. Transaction Type Safety (client-helpers/src/transaction.ts:56-69):
    The TransactionResults type correctly maps each operation to its precise return type, preserving per-position typing.

  2. HTTP Method Selection (src/index.ts:172-193):
    Clean separation between GET (read) and POST/PUT/DELETE (write) operations with appropriate body handling.

  3. Endpoint Normalization (src/index.ts:159-170):
    Robust endpoint validation with clear error messages and proper URL construction.

Recommendations

🔧 Minor Improvements

  1. Add Request Timeout Option:

    export type FetchClientOptions = {
      endpoint: string;
      fetch?: FetchFn;
      timeout?: number; // Add this
    };
  2. Consider Adding Retry Logic Configuration:
    For production resilience, consider exposing retry options.

  3. Enhance Type Safety for Procedures:

    // Instead of (schema as any).procedures
    const procedures = 'procedures' in schema ? schema.procedures : undefined;

Final Verdict

✅ APPROVED - This is a high-quality implementation that:

  • Follows established patterns and conventions
  • Provides excellent type safety and developer experience
  • Has comprehensive test coverage
  • Maintains backward compatibility
  • Adds valuable functionality to the ZenStack ecosystem

The refactoring work is also well-executed and improves code organization without breaking existing functionality.


@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Claude finished @ymc9's task —— View job


PR Review: feat(fetch-client): implement fetch-based CRUD API client

I've completed my comprehensive review of this new fetch-client package. Here's my detailed feedback:

Review Tasks

  • Analyze main fetch-client implementation
  • Review cross-package refactoring changes
  • Examine test coverage and quality
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Provide overall feedback and recommendations

🟢 Overall Assessment: Strong Implementation

This is a well-designed and implemented addition to the ZenStack ecosystem. The code quality is high, follows established patterns, and provides excellent type safety.

Code Quality & Best Practices

Excellent Type Safety

  • Sophisticated use of mapped types to preserve per-call generics (src/index.ts:106-120)
  • Proper constraint propagation through ExtQueryArgs and ExtResult threading
  • Good use of conditional types for optional procedure integration (src/index.ts:94-97)

Clean Architecture

  • Well-separated concerns with distinct functions for read/write operations
  • Consistent naming conventions matching the existing codebase
  • Proper abstraction of common functionality in helper functions

Error Handling

  • Custom CrudError class with typed error codes (src/index.ts:43-55)
  • Proper validation of endpoint URLs with helpful error messages (src/index.ts:159-170)
  • Client-side implementation of *OrThrow methods with appropriate error context

Test Coverage Analysis

Comprehensive Coverage (60 tests)

  • Runtime tests: 47 tests covering all CRUD operations, transactions, procedures, error scenarios
  • Type-level tests: 13 tests ensuring TypeScript inference works correctly
  • Good test organization with separate schemas for different scenarios
  • Proper mocking of fetch requests with realistic response structures

Test Quality

  • Tests verify both HTTP method/URL correctness and response handling
  • Good edge case coverage (empty endpoints, trailing slashes, custom fetch functions)
  • Type-level tests ensure slicing and result narrowing work as expected

Security Considerations

Good Security Practices

  • Endpoint validation prevents relative URL injection (src/index.ts:164-166)
  • Proper JSON marshaling using established utilities
  • No hardcoded credentials or sensitive data
  • Uses existing security patterns from the ecosystem

⚠️ Minor Security Notes

  • Consider adding request timeout configuration for production use
  • The endpoint validation could potentially be bypassed with javascript: or data: URLs, though these would likely fail in practice

Performance Considerations

Efficient Design

  • Lazy construction of model operations in buildModelOperations
  • Reuses established fetch utilities and marshaling logic
  • Sequential transactions are appropriately implemented server-side
  • No unnecessary object creation or complex computations

💡 Performance Opportunities

  • Consider connection pooling options for high-throughput scenarios (could be added to fetch options)
  • The model operations could potentially be memoized if endpoint/fetch config is stable

Cross-Package Refactoring

Well-Executed Refactoring

  • Smart promotion of shared types to @zenstackhq/client-helpers
  • Maintains backward compatibility in tanstack-query through re-exports
  • Clean separation of concerns (moved DEFAULT_QUERY_ENDPOINT to where it belongs)
  • No breaking changes at call sites

Potential Issues Found

🔍 Minor Issues

  1. Type Assertion in Schema Access (src/index.ts:279):

    const procedures = (schema as any).procedures

    Consider using a more type-safe approach or adding a comment explaining why this is necessary.

  2. Missing Input Validation in Procedures (src/index.ts:285-292):
    Custom procedures don't validate input types - though this may be intentional to match server-side validation patterns.

Specific Code Comments

Well-Implemented Features

  1. Transaction Type Safety (client-helpers/src/transaction.ts:56-69):
    The TransactionResults type correctly maps each operation to its precise return type, preserving per-position typing.

  2. HTTP Method Selection (src/index.ts:172-193):
    Clean separation between GET (read) and POST/PUT/DELETE (write) operations with appropriate body handling.

  3. Endpoint Normalization (src/index.ts:159-170):
    Robust endpoint validation with clear error messages and proper URL construction.

Recommendations

🔧 Minor Improvements

  1. Add Request Timeout Option:

    export type FetchClientOptions = {
      endpoint: string;
      fetch?: FetchFn;
      timeout?: number; // Add this
    };
  2. Consider Adding Retry Logic Configuration:
    For production resilience, consider exposing retry options.

  3. Enhance Type Safety for Procedures:

    // Instead of (schema as any).procedures
    const procedures = 'procedures' in schema ? schema.procedures : undefined;

Final Verdict

✅ APPROVED - This is a high-quality implementation that:

  • Follows established patterns and conventions
  • Provides excellent type safety and developer experience
  • Has comprehensive test coverage
  • Maintains backward compatibility
  • Adds valuable functionality to the ZenStack ecosystem

The refactoring work is also well-executed and improves code organization without breaking existing functionality.


@claude
Copy link
Copy Markdown
Contributor

claude Bot commented May 6, 2026

Claude finished @ymc9's task —— View job


PR Review: feat(fetch-client): implement fetch-based CRUD API client

I've completed my comprehensive review of this new fetch-client package. Here's my detailed feedback:

Review Tasks

  • Analyze main fetch-client implementation
  • Review cross-package refactoring changes
  • Examine test coverage and quality
  • Check for potential bugs and security issues
  • Assess performance considerations
  • Provide overall feedback and recommendations

🟢 Overall Assessment: Strong Implementation

This is a well-designed and implemented addition to the ZenStack ecosystem. The code quality is high, follows established patterns, and provides excellent type safety.

Code Quality & Best Practices

Excellent Type Safety

  • Sophisticated use of mapped types to preserve per-call generics (src/index.ts:106-120)
  • Proper constraint propagation through ExtQueryArgs and ExtResult threading
  • Good use of conditional types for optional procedure integration (src/index.ts:94-97)

Clean Architecture

  • Well-separated concerns with distinct functions for read/write operations
  • Consistent naming conventions matching the existing codebase
  • Proper abstraction of common functionality in helper functions

Error Handling

  • Custom CrudError class with typed error codes (src/index.ts:43-55)
  • Proper validation of endpoint URLs with helpful error messages (src/index.ts:159-170)
  • Client-side implementation of *OrThrow methods with appropriate error context

Test Coverage Analysis

Comprehensive Coverage (60 tests)

  • Runtime tests: 47 tests covering all CRUD operations, transactions, procedures, error scenarios
  • Type-level tests: 13 tests ensuring TypeScript inference works correctly
  • Good test organization with separate schemas for different scenarios
  • Proper mocking of fetch requests with realistic response structures

Test Quality

  • Tests verify both HTTP method/URL correctness and response handling
  • Good edge case coverage (empty endpoints, trailing slashes, custom fetch functions)
  • Type-level tests ensure slicing and result narrowing work as expected

Security Considerations

Good Security Practices

  • Endpoint validation prevents relative URL injection (src/index.ts:164-166)
  • Proper JSON marshaling using established utilities
  • No hardcoded credentials or sensitive data
  • Uses existing security patterns from the ecosystem

⚠️ Minor Security Notes

  • Consider adding request timeout configuration for production use
  • The endpoint validation could potentially be bypassed with javascript: or data: URLs, though these would likely fail in practice

Performance Considerations

Efficient Design

  • Lazy construction of model operations in buildModelOperations
  • Reuses established fetch utilities and marshaling logic
  • Sequential transactions are appropriately implemented server-side
  • No unnecessary object creation or complex computations

💡 Performance Opportunities

  • Consider connection pooling options for high-throughput scenarios (could be added to fetch options)
  • The model operations could potentially be memoized if endpoint/fetch config is stable

Cross-Package Refactoring

Well-Executed Refactoring

  • Smart promotion of shared types to @zenstackhq/client-helpers
  • Maintains backward compatibility in tanstack-query through re-exports
  • Clean separation of concerns (moved DEFAULT_QUERY_ENDPOINT to where it belongs)
  • No breaking changes at call sites

Potential Issues Found

🔍 Minor Issues

  1. Type Assertion in Schema Access (src/index.ts:279):

    const procedures = (schema as any).procedures

    Consider using a more type-safe approach or adding a comment explaining why this is necessary.

  2. Missing Input Validation in Procedures (src/index.ts:285-292):
    Custom procedures don't validate input types - though this may be intentional to match server-side validation patterns.

Specific Code Comments

Well-Implemented Features

  1. Transaction Type Safety (client-helpers/src/transaction.ts:56-69):
    The TransactionResults type correctly maps each operation to its precise return type, preserving per-position typing.

  2. HTTP Method Selection (src/index.ts:172-193):
    Clean separation between GET (read) and POST/PUT/DELETE (write) operations with appropriate body handling.

  3. Endpoint Normalization (src/index.ts:159-170):
    Robust endpoint validation with clear error messages and proper URL construction.

Recommendations

🔧 Minor Improvements

  1. Add Request Timeout Option:

    export type FetchClientOptions = {
      endpoint: string;
      fetch?: FetchFn;
      timeout?: number; // Add this
    };
  2. Consider Adding Retry Logic Configuration:
    For production resilience, consider exposing retry options.

  3. Enhance Type Safety for Procedures:

    // Instead of (schema as any).procedures
    const procedures = 'procedures' in schema ? schema.procedures : undefined;

Final Verdict

✅ APPROVED - This is a high-quality implementation that:

  • Follows established patterns and conventions
  • Provides excellent type safety and developer experience
  • Has comprehensive test coverage
  • Maintains backward compatibility
  • Adds valuable functionality to the ZenStack ecosystem

The refactoring work is also well-executed and improves code organization without breaking existing functionality.


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.

1 participant