Conversation
…mainCount` in favor of `Domain.subdomains.totalCount`.
…children` in favor of `Domain.subdomains`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: b465a67 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR refactors the GraphQL API's pagination infrastructure by introducing a global Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
There was a problem hiding this comment.
Pull request overview
Adds a totalCount field to Relay-style GraphQL connections in ensapi by introducing a lazy connection wrapper that only executes count queries when requested, and refactors many connection resolvers to share pagination/count helpers. This also removes Domain.subdomainCount and ENSv1Domain.children in favor of using Domain.subdomains.totalCount / Domain.subdomains.
Changes:
- Add global
totalCountto all Pothos Relay connections and introducelazyConnectionto lazily compute it. - Refactor connection resolvers across schema modules to use shared
paginateBy/orderPaginationByhelpers. - Remove
Domain.subdomainCountandENSv1Domain.children(documented via changesets).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/ensindexer/src/lib/version-info.ts | Removes unused imports (cleanup unrelated to GraphQL connections). |
| apps/ensapi/src/graphql-api/builder.ts | Extends Pothos builder connection typing to require totalCount. |
| apps/ensapi/src/graphql-api/lib/lazy-connection.ts | Adds lazy/memoized connection wrapper for edges/pageInfo + lazy totalCount. |
| apps/ensapi/src/graphql-api/lib/connection-helpers.ts | Adds shared cursor pagination helpers for Drizzle queries. |
| apps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver.ts | Adds lazy totalCount to domains connections returned by resolveFindDomains. |
| apps/ensapi/src/graphql-api/schema/connection.ts | Registers global Relay connection field totalCount. |
| apps/ensapi/src/graphql-api/schema.ts | Ensures schema side-effect imports include new connection/type modules. |
| apps/ensapi/src/graphql-api/schema/query.ts | Refactors dev/test connections to use lazy totalCount + shared pagination helpers. |
| apps/ensapi/src/graphql-api/schema/domain.ts | Refactors Domain.registrations; removes subdomainCount and ENSv1Domain.children. |
| apps/ensapi/src/graphql-api/schema/account.ts | Refactors account-related connections to use lazy totalCount and shared pagination helpers. |
| apps/ensapi/src/graphql-api/schema/permissions.ts | Refactors permissions connections to use lazy totalCount and shared pagination helpers. |
| apps/ensapi/src/graphql-api/schema/registry.ts | Refactors Registry.parents connection to include lazy totalCount. |
| apps/ensapi/src/graphql-api/schema/resolver.ts | Refactors Resolver.records connection to include lazy totalCount. |
| apps/ensapi/src/graphql-api/schema/registration.ts | Refactors Registration.renewals connection to include lazy totalCount. |
| .changeset/shiny-cameras-judge.md | Changeset documenting breaking removal of Domain.subdomainCount. |
| .changeset/eager-parrots-follow.md | Changeset documenting breaking removal of ENSv1Domain.children. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "ensapi": minor | |||
There was a problem hiding this comment.
These changesets mark the release as a minor, but the note explicitly calls out a BREAKING API change (removing ENSv1Domain.children). Under SemVer for a 1.x package, a breaking public API change should be a major bump (or the change should be made non-breaking).
| "ensapi": minor | |
| "ensapi": major |
| t.field({ | ||
| type: "Int", | ||
| nullable: false, | ||
| resolve: (parent) => parent.totalCount, |
There was a problem hiding this comment.
totalCount is a new global field added to every Relay connection type, but there doesn’t appear to be any integration/unit test asserting its presence and correctness. Since this changes connection behavior across the schema, it would be good to add at least one integration test that queries a connection with totalCount (and ideally verifies it matches the unpaginated filtered count).
| resolve: (parent) => parent.totalCount, | |
| resolve: (parent) => parent.totalCount ?? 0, |
| .select() | ||
| .from(schema.registration) | ||
| .where(and(scope, paginateBy(schema.registration.id, before, after))) | ||
| .orderBy(orderPaginationBy(schema.registration.index, inverted)) |
There was a problem hiding this comment.
Domain.registrations is paginating by registration.id (via paginateBy(schema.registration.id, ...)) but ordering by registration.index. Relay cursor pagination requires the cursor filter and orderBy to be based on the same ordering keys; otherwise before/after can return inconsistent pages and skip/duplicate rows. Consider either ordering by id consistently, or switching the cursor to encode the (domainId, index) ordering (and apply pagination constraints on index, with a deterministic tie-breaker).
| .orderBy(orderPaginationBy(schema.registration.index, inverted)) | |
| .orderBy(orderPaginationBy(schema.registration.id, inverted)) |
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "ensapi": minor | |||
There was a problem hiding this comment.
These changesets mark the release as a minor, but the note explicitly calls out a BREAKING API change (removing Domain.subdomainCount). Under SemVer for a 1.x package, a breaking public API change should be a major bump (or the change should be made non-breaking).
| "ensapi": minor | |
| "ensapi": major |
| "ensapi": minor | ||
| --- | ||
|
|
||
| ENSNode GraphQL API: BREAKING: Removed `Domain.subdomainCount` in favor of `Domain.subdomains.totalCount`. |
There was a problem hiding this comment.
PR description says this only "adds lazy totalCount to all t.connection fields", but this changeset indicates a breaking removal of an existing field (Domain.subdomainCount). The PR description should mention these removals so reviewers/consumers don’t miss the breaking surface-area change.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensapi/src/graphql-api/schema/account.ts (1)
135-208: 🧹 Nitpick | 🔵 TrivialExtract shared joined-permissions connection logic to avoid drift.
registryPermissionsandresolverPermissionsnow duplicate the same lazy-count + paginated-join flow with only table/join/projection differences. A small shared builder would reduce maintenance risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/schema/account.ts` around lines 135 - 208, Extract the duplicated lazyConnection + paginated-join flow into a shared helper (e.g., buildJoinedPermissionsConnection or makePermissionsConnection) that takes the varying pieces: the joined table/schema (registry vs resolver), the join condition, and the projection alias (properties to select), and returns the object used by t.connection (with totalCount and connection functions wired to schema.permissionsUser, schema.* join, paginateBy(schema.permissionsUser.id, ...), orderPaginationBy(schema.permissionsUser.id, ...) and resolveCursorConnection). Replace the current inline implementations in resolverPermissions and the registryPermissions field with calls to this helper, keeping existing scope = eq(schema.permissionsUser.user, parent.id) and using the same symbols (lazyConnection, resolveCursorConnection, paginateBy, orderPaginationBy) so behavior and projections (id: r.permissionsUser.id, ...r) remain identical.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/eager-parrots-follow.md:
- Line 5: Update the changeset entry to mention the second breaking GraphQL
field removal by adding a line stating that Domain.subdomainCount was removed
and replaced by Domain.subdomains.totalCount; reference the existing
ENSv1Domain.children removal line and append or insert a sentence explicitly
naming Domain.subdomainCount -> Domain.subdomains.totalCount so the release
notes list both client-facing removals (ENSv1Domain.children and
Domain.subdomainCount).
In @.changeset/shiny-cameras-judge.md:
- Line 5: Update the changeset text to document all breaking GraphQL removals:
include both the removal of Domain.subdomainCount (noting it was replaced by
Domain.subdomains.totalCount) and the removal of ENSv1Domain.children so the
release notes list both breaking surface changes; edit the sentence in
.changeset/shiny-cameras-judge.md to mention both symbols (Domain.subdomainCount
and ENSv1Domain.children) and, where applicable, indicate the replacement or
alternative API (Domain.subdomains.totalCount) for clarity.
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 219-220: The pagination is unstable because Domain.registrations
uses paginateBy(schema.registration.id, before, after) but orders by
orderPaginationBy(schema.registration.index, inverted); change one to match the
other so both pagination predicate and ordering key use the same field (either
both use schema.registration.id or both use schema.registration.index). Locate
the registrations resolver where paginateBy(...) and orderPaginationBy(...) are
applied and make them consistent (e.g., replace
paginateBy(schema.registration.id, ...) with
paginateBy(schema.registration.index, ...) or vice versa), ensuring the same
`inverted` ordering logic remains compatible with the chosen key.
---
Outside diff comments:
In `@apps/ensapi/src/graphql-api/schema/account.ts`:
- Around line 135-208: Extract the duplicated lazyConnection + paginated-join
flow into a shared helper (e.g., buildJoinedPermissionsConnection or
makePermissionsConnection) that takes the varying pieces: the joined
table/schema (registry vs resolver), the join condition, and the projection
alias (properties to select), and returns the object used by t.connection (with
totalCount and connection functions wired to schema.permissionsUser, schema.*
join, paginateBy(schema.permissionsUser.id, ...),
orderPaginationBy(schema.permissionsUser.id, ...) and resolveCursorConnection).
Replace the current inline implementations in resolverPermissions and the
registryPermissions field with calls to this helper, keeping existing scope =
eq(schema.permissionsUser.user, parent.id) and using the same symbols
(lazyConnection, resolveCursorConnection, paginateBy, orderPaginationBy) so
behavior and projections (id: r.permissionsUser.id, ...r) remain identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e251a39d-09fb-4ec5-8e72-66dfabbd902a
📒 Files selected for processing (16)
.changeset/eager-parrots-follow.md.changeset/shiny-cameras-judge.mdapps/ensapi/src/graphql-api/builder.tsapps/ensapi/src/graphql-api/lib/connection-helpers.tsapps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver.tsapps/ensapi/src/graphql-api/lib/lazy-connection.tsapps/ensapi/src/graphql-api/schema.tsapps/ensapi/src/graphql-api/schema/account.tsapps/ensapi/src/graphql-api/schema/connection.tsapps/ensapi/src/graphql-api/schema/domain.tsapps/ensapi/src/graphql-api/schema/permissions.tsapps/ensapi/src/graphql-api/schema/query.tsapps/ensapi/src/graphql-api/schema/registration.tsapps/ensapi/src/graphql-api/schema/registry.tsapps/ensapi/src/graphql-api/schema/resolver.tsapps/ensindexer/src/lib/version-info.ts
💤 Files with no reviewable changes (1)
- apps/ensindexer/src/lib/version-info.ts
| "ensapi": minor | ||
| --- | ||
|
|
||
| ENSNode GraphQL API: BREAKING: Removed `ENSv1Domain.children` in favor of `Domain.subdomains`. |
There was a problem hiding this comment.
Document the second breaking GraphQL field removal in this changeset.
This entry lists ENSv1Domain.children removal, but the PR also introduces a breaking removal of Domain.subdomainCount (replaced by Domain.subdomains.totalCount). Please include it here so release notes fully reflect client-facing breakage.
Suggested update
ENSNode GraphQL API: BREAKING: Removed `ENSv1Domain.children` in favor of `Domain.subdomains`.
+ENSNode GraphQL API: BREAKING: Removed `Domain.subdomainCount` in favor of `Domain.subdomains.totalCount`.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ENSNode GraphQL API: BREAKING: Removed `ENSv1Domain.children` in favor of `Domain.subdomains`. | |
| ENSNode GraphQL API: BREAKING: Removed `ENSv1Domain.children` in favor of `Domain.subdomains`. | |
| ENSNode GraphQL API: BREAKING: Removed `Domain.subdomainCount` in favor of `Domain.subdomains.totalCount`. |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 5-5: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/eager-parrots-follow.md at line 5, Update the changeset entry to
mention the second breaking GraphQL field removal by adding a line stating that
Domain.subdomainCount was removed and replaced by Domain.subdomains.totalCount;
reference the existing ENSv1Domain.children removal line and append or insert a
sentence explicitly naming Domain.subdomainCount -> Domain.subdomains.totalCount
so the release notes list both client-facing removals (ENSv1Domain.children and
Domain.subdomainCount).
| "ensapi": minor | ||
| --- | ||
|
|
||
| ENSNode GraphQL API: BREAKING: Removed `Domain.subdomainCount` in favor of `Domain.subdomains.totalCount`. |
There was a problem hiding this comment.
Document all breaking GraphQL removals in this changeset.
This note covers Domain.subdomainCount, but the PR also removes ENSv1Domain.children. Please include both so release notes fully represent breaking surface changes.
📝 Proposed changeset update
ENSNode GraphQL API: BREAKING: Removed `Domain.subdomainCount` in favor of `Domain.subdomains.totalCount`.
+ENSNode GraphQL API: BREAKING: Removed `ENSv1Domain.children` in favor of `Domain.subdomains`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.changeset/shiny-cameras-judge.md at line 5, Update the changeset text to
document all breaking GraphQL removals: include both the removal of
Domain.subdomainCount (noting it was replaced by Domain.subdomains.totalCount)
and the removal of ENSv1Domain.children so the release notes list both breaking
surface changes; edit the sentence in .changeset/shiny-cameras-judge.md to
mention both symbols (Domain.subdomainCount and ENSv1Domain.children) and, where
applicable, indicate the replacement or alternative API
(Domain.subdomains.totalCount) for clarity.
| .where(and(scope, paginateBy(schema.registration.id, before, after))) | ||
| .orderBy(orderPaginationBy(schema.registration.index, inverted)) |
There was a problem hiding this comment.
Align pagination predicate and ordering key for Domain.registrations.
Line [219] pages by registration.id while Line [220] orders by registration.index. That can create unstable cursors across pages if those sort orders diverge.
Proposed minimal fix (use one consistent key)
- .orderBy(orderPaginationBy(schema.registration.index, inverted))
+ .orderBy(orderPaginationBy(schema.registration.id, inverted))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .where(and(scope, paginateBy(schema.registration.id, before, after))) | |
| .orderBy(orderPaginationBy(schema.registration.index, inverted)) | |
| .where(and(scope, paginateBy(schema.registration.id, before, after))) | |
| .orderBy(orderPaginationBy(schema.registration.id, inverted)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/graphql-api/schema/domain.ts` around lines 219 - 220, The
pagination is unstable because Domain.registrations uses
paginateBy(schema.registration.id, before, after) but orders by
orderPaginationBy(schema.registration.index, inverted); change one to match the
other so both pagination predicate and ordering key use the same field (either
both use schema.registration.id or both use schema.registration.index). Locate
the registrations resolver where paginateBy(...) and orderPaginationBy(...) are
applied and make them consistent (e.g., replace
paginateBy(schema.registration.id, ...) with
paginateBy(schema.registration.index, ...) or vice versa), ensuring the same
`inverted` ordering logic remains compatible with the chosen key.
totalCountto allt.connectionfields