Uh oh!
There was an error while loading. Please reload this page.
perf(orm): speed up client type checking via options projection + variance annotations - #2702
Conversation
…iance annotations The `ClientContract` type graph was dominated by two type-checking costs on non-trivial schemas: 1. The inferred client `Options` literal (heavy `computedFields`/`procedures` function types) was threaded as a type argument into every model's `ModelOperations` (30+ instantiations). 2. TypeScript spent most of the time *measuring* variance of the large, recursive CRUD generics - measurement that comes back "unreliable" and is pure wasted work. Fixes: - Add `QueryRelevantOptions` and project `Options` to its query-relevant subset (`omit`/`slicing`) at the model-map fan-out, so the heavy literal stays out of the per-model types. - Add explicit variance annotations to `ModelOperations`, `CommonModelOperations`, `ZodSchemaFactory`, and the leaf CRUD arg/filter types so the checker skips the expensive (and unreliable) variance measurement. On the taskforge sample (34 models): ~2.78M -> ~0.95M type instantiations and ~4.4s -> ~1.6s check time (~66% / ~64%). Trade-off: `Options` is invariant on the model operations, so a client built with an explicit `omit`/`slicing` literal is no longer assignable to the bare `ClientContract<SchemaDef>` (default options). Schema-agnostic call sites that take `ClientContract<SchemaDef>` should widen via a cast; `proxy.ts` is updated accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR updates ORM TypeScript contracts to use QueryRelevantOptions and explicit variance annotations, centralizes tsx in the pnpm catalog and switches manifests to ChangesORM Type System: Variance & Options Refinements
Dependency Catalog Synchronization
TaskForge Sample Application: Complete CLI Demo
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: TypeScript Performance OptimizationTodo List:
Overall Assessment: ✅ Excellent Performance OptimizationThis is a well-executed performance optimization that addresses real TypeScript compilation bottlenecks with impressive results (~66% reduction in type checking time). Key Strengths🚀 Performance Impact
🎯 Technical ApproachOptions Projection (
Variance Annotations
📚 Code QualityDocumentation
Architecture
🏗️ Sample Quality (TaskForge)
Areas for Consideration
|
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
17-17:⚠️ Potential issue | 🟠 MajorAlign pnpm pin with repository policy.
File: package.json
Lines: 17-17The repo’s root
package.jsoncurrently has:"packageManager": "pnpm@10.33.0",but the policy requires
pnpm@10.12.1.Suggested fix
- "packageManager": "pnpm@10.33.0",+ "packageManager": "pnpm@10.12.1",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 17, Update the packageManager entry value to match repo policy: change the "packageManager" field (the packageManager key in package.json) from "pnpm@10.33.0" to "pnpm@10.12.1" so the manifest pins the required pnpm version.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@samples/taskforge/package.json`:
- Around line 1-31: Add a pinned package manager field to package.json to lock
workspace pnpm version: update the top-level manifest (package.json) to include
"packageManager": "pnpm@10.12.1" so the workspace (scripts like "build", "cli",
and dependencies resolution for `@zenstackhq/`* and better-auth) uses the
documented pnpm version for reproducible installs.
In `@samples/taskforge/README.md`:
- Around line 49-59: The fenced code block that shows the project tree (the
block beginning with ```) is missing a language tag which triggers markdownlint
MD040; update the opening fence from ``` to a tagged fence such as ```text (or
```bash) so the block becomes a language-tagged fenced code block and
markdownlint MD040 is resolved.
- Around line 92-96: The README references the npm script "pnpm auth:generate"
but the project's package.json scripts block doesn't define it; either add a
script named "auth:generate" to package.json (scripts) that invokes the actual
generator CLI used elsewhere (e.g., the same command used for signup like "pnpm
cli auth:generate" or the project's generator entrypoint), or edit the README
text to instruct readers to run the concrete command that works (for example
"pnpm cli auth:generate") instead of "pnpm auth:generate"; update the README.md
or package.json accordingly so the referenced symbol "pnpm auth:generate"
matches an actual runnable script in package.json (scripts).
In `@samples/taskforge/src/auth.ts`:
- Line 24: The hardcoded fallback for the auth signing secret weakens security;
remove the static string fallback from the secret property (the use of
process.env.BETTER_AUTH_SECRET ?? 'taskforge-dev-secret-0123456789abcdef') and
require a real secret at startup instead—read process.env.BETTER_AUTH_SECRET
directly for the secret property and add a startup check that throws or exits
with a clear error if BETTER_AUTH_SECRET is undefined so the application cannot
start with a predictable secret (update any initialization code that references
the secret property accordingly).
In `@samples/taskforge/src/cli.ts`:
- Around line 76-83: The CLI currently selects projects by projectSlug only
which is ambiguous across organizations; update the command signatures that call
db.issue.findMany, db.project.findUnique/update/delete (the CLI
.argument('<projectSlug>' ...) .action handlers) to also accept an organization
identifier (e.g., add an '<orgSlug>' argument or replace projectSlug with a
globally unique projectId) and change the DB filters to scope by both project
slug and organization slug (e.g., project: { slug: projectSlug, organization: {
slug: orgSlug } }) or use projectId in where clauses; apply the same change to
each handler that queries by project slug (the handlers around the
db.issue.findMany and the project read/update/delete calls).
- Line 83: The code is unsafely casting opts.status and opts.priority to bypass
TypeScript validation; instead, validate these CLI inputs against the allowed
enum values before building the query/filter (look for usages of opts.status and
opts.priority in the CLI command handler that construct the object with
...(opts.status ? { status: opts.status as never } : {}) and similar for
priority). Replace the casts with a runtime check that the provided value is one
of the enum members (e.g., compare against Object.values(StatusEnum) or a
predefined allowedValues array) and if invalid, exit or print a clear CLI
validation error; only include the value in the ORM call when the check passes.
Ensure the same change is applied to both spots where status and priority are
cast.
- Around line 195-205: The current approach in db.$transaction using
tx.issue.aggregate to compute next = (max._max.number ?? 0) + 1 before
tx.issue.create is race-prone and will cause unique conflicts on the [projectId,
number] uniqueness; replace this by an atomic increment strategy or add
retry-on-unique-conflict logic: either introduce and increment an atomic
counter/sequence per project (preferred) and use that value for tx.issue.create,
or wrap the create in a short retry loop that catches the unique-constraint
error (e.g., Prisma P2002) and retries the transaction to recompute the next
number; locate the logic in the db.$transaction block where tx.issue.aggregate
and tx.issue.create are used and implement one of these fixes (atomic
counter/sequence per project or retry-on-P2002) to guarantee unique,
collision-free numbering.
In `@samples/taskforge/zenstack/schema.zmodel`:
- Around line 509-518: The Reaction model allows both or neither target because
issueId and commentId are nullable; fix by enforcing XOR: add a model-level
constraint that exactly one target is set (e.g., add @@check("(CASE WHEN issueId
IS NOT NULL THEN 1 ELSE 0 END) + (CASE WHEN commentId IS NOT NULL THEN 1 ELSE 0
END) = 1") or the equivalent dialect-specific check) referencing the fields
issueId and commentId, or alternatively split the model into two distinct models
(IssueReaction and CommentReaction) with their own foreign key and unique
constraints; update any create/update logic to satisfy the new constraint.
---
Outside diff comments:
In `@package.json`:
- Line 17: Update the packageManager entry value to match repo policy: change
the "packageManager" field (the packageManager key in package.json) from
"pnpm@10.33.0" to "pnpm@10.12.1" so the manifest pins the required pnpm version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fb9dd35b-ae38-4508-a140-7f179a8984af
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
package.jsonpackages/cli/src/actions/proxy.tspackages/orm/package.jsonpackages/orm/src/client/contract.tspackages/orm/src/client/crud-types.tspackages/orm/src/client/options.tspackages/orm/src/client/zod/factory.tspnpm-workspace.yamlsamples/sveltekit/package.jsonsamples/taskforge/README.mdsamples/taskforge/package.jsonsamples/taskforge/src/auth.tssamples/taskforge/src/cli.tssamples/taskforge/src/db.tssamples/taskforge/src/seed.tssamples/taskforge/tsconfig.jsonsamples/taskforge/zenstack/input.tssamples/taskforge/zenstack/models.tssamples/taskforge/zenstack/schema.tssamples/taskforge/zenstack/schema.zmodel
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Extends the variance annotations to `ToOneRelationUpdateInput`, `FilterArgs`, `SortAndTakeArgs`, and `ZodSchemaFactory`, whose `Options` parameter was still being measured (the largest remaining variance cost). The client is already invariant in `Options`, so this introduces no new assignability constraints. Annotating `ToOneRelationUpdateInput` also short-circuits measurement of its conditional children (`DisconnectInput`/`NestedDeleteInput`), which can't carry annotations directly. taskforge sample: ~0.95M -> ~0.55M instantiations, ~1.6s -> ~1.0s check time (cumulative from baseline: ~2.78M -> ~0.55M, ~4.4s -> ~1.0s). Validated: all ORM-dependent packages typecheck clean; e2e client-api (618), omit/slicing/computed (74), and type-level (45) suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude finished @ymc9's task —— View job PR Review: TypeScript Performance OptimizationTodo List:
Overall Assessment: ✅ Excellent Performance OptimizationThis is a well-executed performance optimization that addresses real TypeScript compilation bottlenecks with impressive results (~66% reduction in type checking time). Key Strengths🚀 Performance Impact
🎯 Technical ApproachOptions Projection (
Variance Annotations
📚 Code QualityDocumentation
Architecture
🏗️ Sample Quality (TaskForge)
Areas for Consideration
|
…e holds Making `Options` invariant on the leaf arg types (previous commit) regressed client assignability: `ZodSchemaFactory` received the *unprojected* options via `$zod`, so it produced `FilterArgs<…, fullOptions>`, and the invariant `Options` then required the heavy options literal (carrying `dialect`) to match exactly - breaking `ClientContract<S, Literal>` -> `ClientContract<S>` (e.g. the taskforge sample's `db: DB`). Fix: pass the client's *query-relevant* (projected) options to `ZodSchemaFactory` via the `$zod` accessor, relax its bound to `QueryOptions`, erase the client `Options` arg in its constructor, and read the runtime-only `plugins` field weakly. Now every path that instantiates the leaf arg types feeds them projected options, so their invariance no longer constrains the options literal. taskforge sample: 0 errors, ~0.55M instantiations, ~1.0s. All ORM-dependent packages typecheck clean; zod/omit/slicing (257) and type-level (45) suites pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude finished @ymc9's task —— View job PR Review: TypeScript Performance OptimizationTodo List:
Overall Assessment: ✅ Excellent Performance OptimizationThis is a well-executed performance optimization that addresses real TypeScript compilation bottlenecks with impressive results (~66% reduction in type checking time). Key Strengths🚀 Performance Impact
🎯 Technical ApproachOptions Projection (
Variance Annotations
📚 Code QualityDocumentation
Architecture
🏗️ Sample Quality (TaskForge)
Areas for Consideration
|
Claude finished @ymc9's task —— View job PR Review: TypeScript Performance OptimizationTodo List:
Overall Assessment: ✅ Excellent Performance OptimizationThis is a well-executed performance optimization that addresses real TypeScript compilation bottlenecks with impressive results (~66% reduction in type checking time). Key Strengths🚀 Performance Impact
🎯 Technical ApproachOptions Projection (
Variance Annotations
📚 Code QualityDocumentation
Architecture
🏗️ Sample Quality (TaskForge)
Areas for Consideration
|
Summary
Speeds up type checking of the ZenStack client (
ClientContract) on non-trivial schemas. On the includedtaskforgesample (34 models): ~2.78M → ~0.95M type instantiations and ~4.4s → ~1.6stsccheck time (~66% / ~64% reduction).Root cause
Two costs dominated
tscon the client type:new ZenStackClient(...)infers anOptionsliteral carryingcomputedFields/proceduresfunction types; that literal was passed as a type argument into every model'sModelOperations(30+ instantiations)."unreliable"and then falls back to full structural comparison anyway.Changes
options.ts— addQueryRelevantOptions<Schema, Options>(theomit/slicingsubset of options — the only parts that influence types).contract.ts— projectOptionstoQueryRelevantOptionsat the model-map fan-out (keeps the heavy literal out of per-model types), and add explicit variance annotations toClientContract'sModelOperations/CommonModelOperationsso the checker skips the expensive variance measurement.crud-types.ts— variance annotations on the leaf CRUD arg/filter types.zod/factory.ts— variance annotation onZodSchemaFactory.cli/.../proxy.ts— see trade-off below.Trade-off
Optionsis invariant on the model operations (it's read foromit/slicingin both arg and result positions). As a result, a client constructed with an explicitomit/slicingliteral is no longer assignable to the bareClientContract<SchemaDef>(default options). Schema-agnostic call sites that acceptClientContract<SchemaDef>should widen via a cast —proxy.tsis updated accordingly, and the constraint is documented in-code.Validation
orm,server,sdk,cli,plugins/*,auth-adapters/*,clients/*).taskforgesample: 0 type errors.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores