ENSv2 Find Domain Ordering Support #1595
Conversation
🦋 Changeset detectedLatest commit: 618d7a8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces ordered, cursor-based pagination for domain queries: new OrderDirection and DomainsOrderBy enums, DomainsOrderInput, DomainCursor encoding/decoding, centralized resolveFindDomains resolver, rebuilt findDomains query builders/types, and schema updates to Query.domains and Account.domains to accept an order argument. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQL as GraphQL Resolver
participant ResolveFind as resolveFindDomains
participant Cursor as DomainCursor
participant Builder as findDomains / orderFindDomains
participant DB as Database
participant DL as DomainDataLoader
Client->>GraphQL: Query domains(order, first/after)
GraphQL->>ResolveFind: resolveFindDomains({ where, order, first, after })
ResolveFind->>Cursor: decode(after)
Cursor-->>ResolveFind: DomainCursor (id, by, dir, value)
ResolveFind->>Builder: build base union query + cursor filter + ordering
Builder->>DB: Execute paginated SQL
DB-->>ResolveFind: Rows with order values
ResolveFind->>DL: load(ids...)
DL-->>ResolveFind: Hydrated Domain objects
ResolveFind->>Cursor: encode(...) for edges
ResolveFind-->>GraphQL: Connection (edges, pageInfo)
GraphQL-->>Client: GraphQL response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes 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 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
This PR adds support for ordering domain search results in the ENSv2 API. It introduces new GraphQL input types to allow clients to specify the field to order by (NAME, REGISTRATION_TIMESTAMP, or REGISTRATION_EXPIRY) and the sort direction (ASC or DESC).
Changes:
- Added
OrderDirectionenum andDomainsOrderByenum withDomainsOrderInputtype for specifying ordering in domain queries - Refactored
findDomainsfunction to join additional data (labels and registrations) needed for ordering - Implemented
orderFindDomainsfunction to build SQL ORDER BY clauses based on user input - Integrated ordering support into both
Query.domainsandAccount.domainsfields
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/ensapi/src/graphql-api/schema/order-direction.ts | New file defining OrderDirection enum (ASC/DESC) |
| apps/ensapi/src/graphql-api/schema/domain.ts | Added DomainsOrderBy enum and DomainsOrderInput type, imported OrderDirection |
| apps/ensapi/src/graphql-api/schema/query.ts | Added order parameter to domains query, integrated orderFindDomains function, removed unused asc/desc imports |
| apps/ensapi/src/graphql-api/schema/account.ts | Added order parameter to Account.domains field, integrated orderFindDomains function |
| apps/ensapi/src/graphql-api/lib/find-domains.ts | Refactored findDomains to support ordering with additional joins for labels and registrations, added orderFindDomains function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
shrugs
force-pushed
the
feat/domain-ordering
branch
from
f7b2e8c to
0a03e2a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Fix correlated subquery bug in latestRegistration that compared domainId to itself instead of outer query - Fix partial filtering to use headLabel alias instead of schema.label - Use NULLS LAST for both sort directions so unregistered domains always appear at end Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ain methods, i.e. `Account.domains(order: { by: NAME dir: ASC })`.
…rsors cast cursor.value to ::text or ::bigint in tuple comparison to avoid Postgres "could not determine data type of parameter" errors. also wrap DomainCursor.decode in try/catch for clear error on malformed input. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * The criteria by which the set is ordered. One of NAME, REGISTRATION_TIMESTAMP, or REGISTRATION_EXPIRY. | ||
| */ | ||
| by: typeof DomainsOrderBy.$inferType; | ||
|
|
||
| /** | ||
| * The direction in which the set is ordered, either ASC or DESC. | ||
| */ | ||
| dir: typeof OrderDirection.$inferType; | ||
|
|
There was a problem hiding this comment.
DomainsOrderBy/OrderDirection are imported as type-only, but the DomainCursor interface uses typeof DomainsOrderBy.$inferType and typeof OrderDirection.$inferType (requires value-space access). This should be changed to use DomainsOrderByValue / OrderDirectionValue types, or import the enum refs as values.
| export type DomainsOrderByValue = typeof DomainsOrderBy.$inferType; | ||
|
|
||
| export const DomainsOrderInput = builder.inputType("DomainsOrderInput", { | ||
| description: "Ordering options for domains query. If no order is provided, the default isASC.", |
There was a problem hiding this comment.
Typo in description: "default isASC" is missing a space and reads incorrectly. Please update the description string (e.g., "default is ASC").
| description: "Ordering options for domains query. If no order is provided, the default isASC.", | |
| description: "Ordering options for domains query. If no order is provided, the default is ASC.", |
| export function cursorFilter( | ||
| domains: ReturnType<typeof findDomains>, | ||
| cursor: DomainCursor, | ||
| queryOrderBy: typeof DomainsOrderBy.$inferType, | ||
| queryOrderDir: typeof OrderDirection.$inferType, | ||
| direction: "after" | "before", | ||
| effectiveDesc: boolean, | ||
| ): SQL { |
There was a problem hiding this comment.
There are no tests covering the keyset pagination filter logic in cursorFilter (NULL cursor branch and non-NULL tuple comparison branches). Since this logic is easy to regress and can silently skip/duplicate rows, adding unit tests for the main branches (per orderBy, per direction, NULL/non-NULL cursor.value) would materially reduce risk.
| import type { DomainsOrderBy } from "@/graphql-api/schema/domain"; | ||
| import type { OrderDirection } from "@/graphql-api/schema/order-direction"; |
There was a problem hiding this comment.
DomainsOrderBy and OrderDirection are imported with import type, but later the file uses typeof ...$inferType in signatures. That pattern requires a value import and will fail TS compilation. Consider switching these signatures to the already-exported DomainsOrderByValue / OrderDirectionValue types (or import the enum refs as runtime values).
| import type { DomainsOrderBy } from "@/graphql-api/schema/domain"; | |
| import type { OrderDirection } from "@/graphql-api/schema/order-direction"; | |
| import { DomainsOrderBy } from "@/graphql-api/schema/domain"; | |
| import { OrderDirection } from "@/graphql-api/schema/order-direction"; |
| timestamp: t.field({ | ||
| description: "TODO", | ||
| type: "BigInt", | ||
| nullable: false, | ||
| resolve: (parent) => parent.timestamp, | ||
| }), |
There was a problem hiding this comment.
New Event.timestamp field is exposed but its description is still the placeholder "TODO". Since this is a public schema field, consider giving it a concrete description (units, source, and whether it’s block timestamp).
🚀 Preview Packages -
|
1 participant
closes #1564
Reviewer Focus (Read This First)
cursorFilterinfind-domains.ts— NULL-handling logic for keyset pagination. NULLs can't participate in tuple comparison in Postgres, so they're handled explicitly. explicit::text/::bigintcasts on cursor values to avoidparameter type inference failures in tuple context.
DomainCursordesign — cursor encodesby/diralongside the sort column value. allows cursor validation (stale cursors with mismatched order args throw) and enables tuple comparison without subqueries. uses superjson forbigint encoding; decode is wrapped in try/catch for malformed input.
findDomainsCTE structure — unions v1 and v2 domains, joins head label for partial matching + NAME ordering, joins latest registration for REGISTRATION_* ordering. verify the LEFT JOINs and NULLS LAST behavior.Problem & Motivation
Query.domainsandAccount.domainsonly supported ordering byid, making search/autocomplete UX hard to buildNAME,REGISTRATION_TIMESTAMP, andREGISTRATION_EXPIRYWhat Changed (Concrete)
DomainsOrderByenum (NAME | REGISTRATION_TIMESTAMP | REGISTRATION_EXPIRY) andDomainsOrderInputinput typeOrderDirectionenum (ASC | DESC)orderarg added toQuery.domainsandAccount.domainsconnectionsDomainCursor— composite keyset cursor encoding{ id, by, dir, value }as base64 superjson. decode catches malformed input with a clear error.findDomainsrefactored into a CTE-based query builder. NAME ordering uses the head label (the varying part when a concrete path is specified; equals the leaf when there's no concrete path).find-domains-by-labelhash-pathextracted to its own file (no behavior change)resolveFindDomainsshared resolver replaces duplicated inline pagination logic inquery.tsandaccount.tscursorFilterhandles NULL and non-NULL cursor values, with explicit::text/::bigintcasts in tuple comparisonsEvent.timestampfield added (description is "TODO")superjsondep addedDesign & Planning
composite cursor required because simple id-based cursors break when ordering by non-unique columns
cursor encodes
by/dirto detect stale cursors when callers change order args mid-paginationsuperjson chosen over custom bigint serializer since it already handles the type safely
NULLS LAST is intentional: unregistered domains (NULL registration fields) sort to the end regardless of direction
NAME ordering uses head label rather than leaf label — when a concrete path is specified (e.g.
sub1.sub2.exam), all results share the same leaf, making leaf-based sorting meaningless. head label is the varying part. when there's noconcrete path, head = leaf = self, so it's equivalent.
full canonical name sorting was considered but deferred — requires a second recursive CTE per result and the canonical name isn't materialized. head label + id tiebreaker is sufficient for autocomplete.
Planning artifacts: none
Reviewed / approved by: n/a
Self-Review
fixed cursor pagination bugs with NULL values and direction mismatch after initial draft. caught that leaf label was wrong for NAME ordering (identical across all results when concrete path is specified). added explicit type casts in tuple
comparison to avoid Postgres parameter type inference failures. wrapped cursor decode in try/catch.
resolveFindDomainsto DRYquery.tsandaccount.tsDomainCursorinterface/namespace dual export patternCross-Codebase Alignment
verified
pothos/plugin-relayresolveCursorConnectioncontract (inverted,before,after,limit). reviewed existingcursorsutil (still used elsewhere, not removed).find-domains-by-labelhash-pathlogic unchanged from priorimplementation.
resolveCursorConnection,findDomains,DomainCursor,cursors.decodecursors.ts(still used by other resolvers)Downstream & Consumer Impact
callers: new optional
orderarg onQuery.domainsandAccount.domains; defaults toNAME ASCif omitted. existing queries unaffected.Public APIs affected:
Query.domains(order:),Account.domains(order:)Docs updated: changeset added (
ensapiminor)Naming decisions:
DomainsOrderByvsOrderBy— scoped to avoid collision with future ordering enums on other typesTesting Evidence
DomainCursorencode/decode, andisEffectiveDesccursorFilter, NULL cursor edge cases, cursor decode error handling, or cursor validation errorscursorFilterand theisEffectiveDescXOR logicScope Reductions
cursor format validation (decode currently trusts cursor structure after base64/superjson parse) — tracked as TODO in
domain-cursor.tsreplace correlated registration subquery with a lookup table join — tracked as TODO in
find-domains.ts(ENSv2 Refactor Registration/Renewal Superceding Logic #1594)full canonical name sorting (would require second recursive CTE; head label + id tiebreaker is sufficient for now)
tests for pagination logic
Why they were deferred: keep PR reviewable; lookup table is a separate infrastructure change; full name sorting adds real query cost for marginal UX benefit
Risk Analysis
NULL handling in
cursorFilteris hand-rolled; if the logic is wrong, pagination silently skips or duplicates rowscursor format change is a breaking change for any client that persists cursors across deploys
Risk areas:
cursorFilterNULL branches, cursor invalidation on deployMitigations: cursor validation throws on
by/dirmismatch; explicit casts prevent type inference failures; NULLS LAST is consistent; malformed cursors return clear errorsNamed owner if this causes problems: @shrugs
Pre-Review Checklist (Blocking)