ENSv2 Find Domain Ordering Support #1595
Conversation
🦋 Changeset detectedLatest commit: ee9498c 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
📝 WalkthroughWalkthroughUnifies ENSv1/v2 domain lookup and adds keyset ordering/pagination: new GraphQL order types and cursor helpers, a centralized resolveFindDomains resolver, extended find-domains SQL with ordering helpers (cursorFilter, orderFindDomains), and schema fields updated to accept an order argument. Changes
Sequence DiagramsequenceDiagram
participant Client as GraphQL Client
participant Resolver as Account/Query Resolver
participant ResolverFn as resolveFindDomains
participant Finder as findDomains()
participant DB as Database
participant Loader as Domain DataLoader
Client->>Resolver: query domains(order:{by,dir}, before/after, first/last)
Resolver->>ResolverFn: resolveFindDomains(args, where, order, cursors)
ResolverFn->>Finder: build unified domainsBase + latestRegistration + cursorFilter
Finder->>DB: SQL query (CTE -> join -> ordered rows)
DB-->>Finder: FindDomainsResult rows (id, leafLabelValue, registrationStart, registrationExpiry)
Finder-->>ResolverFn: rows
ResolverFn->>Loader: load full Domain entities by id
Loader-->>ResolverFn: Domain objects
ResolverFn->>Resolver: Domains with attached __orderValue and encoded cursors
Resolver-->>Client: paginated domains response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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.
| const latestRegistration = db | ||
| .select({ | ||
| domainId: schema.registration.domainId, | ||
| start: schema.registration.start, | ||
| expiry: schema.registration.expiry, | ||
| }) | ||
| .from(schema.registration) | ||
| .where( | ||
| eq( | ||
| schema.registration.index, | ||
| db | ||
| .select({ maxIndex: sql<number>`MAX(${schema.registration.index})` }) | ||
| .from(schema.registration) | ||
| .where(eq(schema.registration.domainId, schema.registration.domainId)), | ||
| ), | ||
| ) | ||
| .as("latestRegistration"); |
There was a problem hiding this comment.
The subquery structure for finding the latest registration has performance concerns. Even if the correlation bug on line 156 is fixed, this approach uses a correlated subquery within a WHERE clause (lines 151-157) which will be evaluated for each row in the registration table. This can be extremely slow for large datasets.
A more efficient approach would be to use a window function like ROW_NUMBER() OVER (PARTITION BY domainId ORDER BY index DESC) to find the latest registration, or to use a JOIN with a GROUP BY/MAX subquery. The TODO comment on line 141 acknowledges this should be replaced with a lookup table join for better performance.
![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 })`.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ensapi/src/graphql-api/lib/find-domains.ts (1)
103-127: 🧹 Nitpick | 🔵 TrivialAvoid duplicate owner/partial filters on the v1 base query.
v1DomainsBasefilters by owner/partial and joins labels, but the unifieddomainsquery applies the same filters again viaheadLabel. This duplicates work for v1 only and makes the v1/v2 paths inconsistent. Consider removing the v1-only filters (or pushing the same filters into v2) so filtering happens exactly once.♻️ Suggested simplification
- .innerJoin(v1HeadDomain, eq(v1HeadDomain.id, v1DomainsByLabelHashPathQuery.headId)) - .leftJoin(schema.label, eq(schema.label.labelHash, v1HeadDomain.labelHash)) - .where( - and( - owner ? eq(schema.v1Domain.ownerId, owner) : undefined, - // TODO: determine if it's necessary to additionally escape user input for LIKE operator - // Note: if label is NULL (unlabeled domain), LIKE returns NULL and filters out the row. - // This is intentional - we can't match partial text against unknown labels. - partial ? like(schema.label.interpreted, `${partial}%`) : undefined, - ), - ); + .innerJoin(v1HeadDomain, eq(v1HeadDomain.id, v1DomainsByLabelHashPathQuery.headId));apps/ensapi/src/graphql-api/schema/query.ts (1)
53-83:⚠️ Potential issue | 🟠 MajorCursor filters must follow the active order key.
With
order.byset to NAME, REGISTRATION_TIMESTAMP, or REGISTRATION_EXPIRY, the query still appliesbefore/afterconstraints only ondomains.id. This breaks keyset pagination because when the primary sort column has duplicate values, the cursor no longer corresponds to the ordered result set, causing missed or duplicated rows.Implement either:
- Composite cursor encoding (orderColumn + id) with tuple-style comparisons respecting the effective sort direction
- Restrict ordering to id until cursor handling is updated
This affects both
Query.domainsandAccount.domains.
- Fix correlated subquery bug in latestRegistration (use registrationOuter alias) - Fix partial filtering to use correct table alias (headLabel.interpreted) - Fix NULLS ordering to always use NULLS LAST regardless of sort direction - Remove duplicate v1 filtering from v1DomainsBase (filters applied in unified query) - Implement composite cursor pagination with order value for stable keyset pagination - Add cursor orderBy validation to catch cursor/query mismatch errors - DRY up code: add DomainWithOrderValue type, getOrderValueFromResult helper - Fix changeset syntax (add missing comma) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts`:
- Around line 44-56: The switch in getOrderValueFromResult currently omits a
default/exhaustiveness check for DomainsOrderBy; add an exhaustive type guard so
new enum values fail at compile time (or runtime) by handling the default
branch: assert the remaining `orderBy` as `never` (or call an assertUnreachable
helper) and throw or return a sensible fallback; reference the
getOrderValueFromResult function and the DomainsOrderBy/DomainOrderValue types
when adding the default case to ensure type-checking will catch any future
unhandled cases.
- Around line 410-438: The tuple comparison in cursorFilter will break when
cursorValue is null because PostgreSQL tuple comparisons with NULL produce
unknown; modify cursorFilter to handle cursorValue === null explicitly: call
getOrderColumn as now, compute useGreaterThan/op, then branch — if cursorValue
=== null produce SQL that compares nullness and id instead of a tuple (e.g., for
"after" when ordering ASC: rows with orderColumn IS NOT NULL should be
considered greater than NULL, and for rows where orderColumn IS NULL compare
only the id), otherwise keep the existing tuple comparison; update generated SQL
to combine IS NULL / IS NOT NULL checks with the id comparison (reference
cursorFilter, getOrderColumn, domains.id, DomainsOrderBy) so NULL cursor values
are handled deterministically.
In `@apps/ensapi/src/graphql-api/schema/domain.ts`:
- Around line 375-378: DomainCursor.encode/decode currently use
JSON.stringify/parse which throws on bigint values (DomainOrderValue can be
bigint for REGISTRATION_TIMESTAMP and REGISTRATION_EXPIRY); update
DomainCursor.encode and DomainCursor.decode to serialize bigints safely by using
a custom JSON replacer that converts bigint to a string with a clear marker
(e.g. prefix or wrapper) and a matching reviver in decode that detects that
marker and reconstructs the bigint, or alternately explicitly convert those
known fields (REGISTRATION_TIMESTAMP / REGISTRATION_EXPIRY) to string before
encoding and back to bigint after decoding; ensure the changes touch the
DomainCursor.encode and DomainCursor.decode functions and preserve type
assertions to DomainCursor after reviving.
In `@apps/ensapi/src/graphql-api/schema/query.ts`:
- Around line 80-143: Extract the repeated cursor-pagination flow into a shared
helper (e.g., paginateWithCursors) that takes ResolveCursorConnectionArgs and
the domain-context pieces you already compute (domains, orderBy, orderDir,
inverted, context, db) and encapsulates DomainCursor.decode calls, cursorFilter
construction, orderFindDomains usage, the DB query orchestration, mapping
results to orderValueById via getOrderValueFromResult, loading with
DomainInterfaceRef.getDataloader + rejectAnyErrors, and attaching __orderValue;
then replace the duplicated body in both Query.domains and Account.domains with
a call to that helper so both use the same pagination implementation and reduce
duplication.
| export function getOrderValueFromResult( | ||
| result: FindDomainsResult, | ||
| orderBy: typeof DomainsOrderBy.$inferType, | ||
| ): DomainOrderValue { | ||
| switch (orderBy) { | ||
| case "NAME": | ||
| return result.leafLabelValue; | ||
| case "REGISTRATION_TIMESTAMP": | ||
| return result.registrationStart; | ||
| case "REGISTRATION_EXPIRY": | ||
| return result.registrationExpiry; | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding exhaustiveness check for type safety.
The switch statement handles all current DomainsOrderBy values, but lacks a default case to catch future additions at compile time.
♻️ Proposed fix for exhaustiveness
export function getOrderValueFromResult(
result: FindDomainsResult,
orderBy: typeof DomainsOrderBy.$inferType,
): DomainOrderValue {
switch (orderBy) {
case "NAME":
return result.leafLabelValue;
case "REGISTRATION_TIMESTAMP":
return result.registrationStart;
case "REGISTRATION_EXPIRY":
return result.registrationExpiry;
+ default:
+ // Exhaustiveness check - TypeScript will error if a case is missing
+ const _exhaustive: never = orderBy;
+ throw new Error(`Unhandled orderBy: ${_exhaustive}`);
}
}🤖 Prompt for AI Agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts` around lines 44 - 56, The
switch in getOrderValueFromResult currently omits a default/exhaustiveness check
for DomainsOrderBy; add an exhaustive type guard so new enum values fail at
compile time (or runtime) by handling the default branch: assert the remaining
`orderBy` as `never` (or call an assertUnreachable helper) and throw or return a
sensible fallback; reference the getOrderValueFromResult function and the
DomainsOrderBy/DomainOrderValue types when adding the default case to ensure
type-checking will catch any future unhandled cases.
| export function cursorFilter( | ||
| domains: ReturnType<typeof findDomains>, | ||
| cursorId: DomainId, | ||
| cursorValue: DomainOrderValue | undefined, | ||
| cursorOrderBy: typeof DomainsOrderBy.$inferType, | ||
| queryOrderBy: typeof DomainsOrderBy.$inferType, | ||
| direction: "after" | "before", | ||
| effectiveDesc: boolean, | ||
| ): SQL { | ||
| // Validate cursor was created with the same ordering as the current query | ||
| if (cursorOrderBy !== queryOrderBy) { | ||
| throw new Error( | ||
| `Invalid cursor: cursor was created with orderBy=${cursorOrderBy} but query uses orderBy=${queryOrderBy}`, | ||
| ); | ||
| } | ||
|
|
||
| const orderColumn = getOrderColumn(domains, cursorOrderBy); | ||
|
|
||
| // Determine comparison direction: | ||
| // - "after" with ASC = greater than cursor | ||
| // - "after" with DESC = less than cursor | ||
| // - "before" with ASC = less than cursor | ||
| // - "before" with DESC = greater than cursor | ||
| const useGreaterThan = (direction === "after") !== effectiveDesc; | ||
| const op = useGreaterThan ? ">" : "<"; | ||
|
|
||
| // Direct tuple comparison with cursor values (no subquery needed) | ||
| return sql`(${orderColumn}, ${domains.id}) ${sql.raw(op)} (${cursorValue}, ${cursorId})`; | ||
| } |
There was a problem hiding this comment.
Tuple comparison with NULL values may produce unexpected results.
When cursorValue is null (e.g., for unregistered domains ordered by REGISTRATION_TIMESTAMP), the SQL tuple comparison (orderColumn, id) > (NULL, cursorId) will not behave as expected in PostgreSQL. Comparisons with NULL always yield NULL (unknown), causing these rows to be filtered out incorrectly.
Consider handling the NULL case explicitly with IS NULL / IS NOT NULL logic, or document that ordering by timestamp/expiry only works correctly for domains that have registrations.
🐛 Sketch of a potential fix
export function cursorFilter(
domains: ReturnType<typeof findDomains>,
cursorId: DomainId,
cursorValue: DomainOrderValue | undefined,
cursorOrderBy: typeof DomainsOrderBy.$inferType,
queryOrderBy: typeof DomainsOrderBy.$inferType,
direction: "after" | "before",
effectiveDesc: boolean,
): SQL {
// ... validation ...
const orderColumn = getOrderColumn(domains, cursorOrderBy);
const useGreaterThan = (direction === "after") !== effectiveDesc;
const op = useGreaterThan ? ">" : "<";
+ // Handle NULL cursor values specially - NULL comparisons don't work in tuples
+ if (cursorValue === null || cursorValue === undefined) {
+ // When cursor value is NULL, we need to compare by id only for rows
+ // where orderColumn IS NULL, or include all non-NULL rows depending on direction
+ const nullsAtEnd = true; // NULLS LAST
+ if ((useGreaterThan && nullsAtEnd) || (!useGreaterThan && !nullsAtEnd)) {
+ // Looking for rows after a NULL (at the end) - only compare by id within NULLs
+ return sql`(${orderColumn} IS NULL AND ${domains.id} ${sql.raw(op)} ${cursorId})`;
+ } else {
+ // Looking for rows before a NULL - include all non-NULL rows plus id comparison within NULLs
+ return sql`(${orderColumn} IS NOT NULL OR (${orderColumn} IS NULL AND ${domains.id} ${sql.raw(op)} ${cursorId}))`;
+ }
+ }
+
return sql`(${orderColumn}, ${domains.id}) ${sql.raw(op)} (${cursorValue}, ${cursorId})`;
}📝 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.
| export function cursorFilter( | |
| domains: ReturnType<typeof findDomains>, | |
| cursorId: DomainId, | |
| cursorValue: DomainOrderValue | undefined, | |
| cursorOrderBy: typeof DomainsOrderBy.$inferType, | |
| queryOrderBy: typeof DomainsOrderBy.$inferType, | |
| direction: "after" | "before", | |
| effectiveDesc: boolean, | |
| ): SQL { | |
| // Validate cursor was created with the same ordering as the current query | |
| if (cursorOrderBy !== queryOrderBy) { | |
| throw new Error( | |
| `Invalid cursor: cursor was created with orderBy=${cursorOrderBy} but query uses orderBy=${queryOrderBy}`, | |
| ); | |
| } | |
| const orderColumn = getOrderColumn(domains, cursorOrderBy); | |
| // Determine comparison direction: | |
| // - "after" with ASC = greater than cursor | |
| // - "after" with DESC = less than cursor | |
| // - "before" with ASC = less than cursor | |
| // - "before" with DESC = greater than cursor | |
| const useGreaterThan = (direction === "after") !== effectiveDesc; | |
| const op = useGreaterThan ? ">" : "<"; | |
| // Direct tuple comparison with cursor values (no subquery needed) | |
| return sql`(${orderColumn}, ${domains.id}) ${sql.raw(op)} (${cursorValue}, ${cursorId})`; | |
| } | |
| export function cursorFilter( | |
| domains: ReturnType<typeof findDomains>, | |
| cursorId: DomainId, | |
| cursorValue: DomainOrderValue | undefined, | |
| cursorOrderBy: typeof DomainsOrderBy.$inferType, | |
| queryOrderBy: typeof DomainsOrderBy.$inferType, | |
| direction: "after" | "before", | |
| effectiveDesc: boolean, | |
| ): SQL { | |
| // Validate cursor was created with the same ordering as the current query | |
| if (cursorOrderBy !== queryOrderBy) { | |
| throw new Error( | |
| `Invalid cursor: cursor was created with orderBy=${cursorOrderBy} but query uses orderBy=${queryOrderBy}`, | |
| ); | |
| } | |
| const orderColumn = getOrderColumn(domains, cursorOrderBy); | |
| // Determine comparison direction: | |
| // - "after" with ASC = greater than cursor | |
| // - "after" with DESC = less than cursor | |
| // - "before" with ASC = less than cursor | |
| // - "before" with DESC = greater than cursor | |
| const useGreaterThan = (direction === "after") !== effectiveDesc; | |
| const op = useGreaterThan ? ">" : "<"; | |
| // Handle NULL cursor values specially - NULL comparisons don't work in tuples | |
| if (cursorValue === null || cursorValue === undefined) { | |
| // When cursor value is NULL, we need to compare by id only for rows | |
| // where orderColumn IS NULL, or include all non-NULL rows depending on direction | |
| const nullsAtEnd = true; // NULLS LAST | |
| if ((useGreaterThan && nullsAtEnd) || (!useGreaterThan && !nullsAtEnd)) { | |
| // Looking for rows after a NULL (at the end) - only compare by id within NULLs | |
| return sql`(${orderColumn} IS NULL AND ${domains.id} ${sql.raw(op)} ${cursorId})`; | |
| } else { | |
| // Looking for rows before a NULL - include all non-NULL rows plus id comparison within NULLs | |
| return sql`(${orderColumn} IS NOT NULL OR (${orderColumn} IS NULL AND ${domains.id} ${sql.raw(op)} ${cursorId}))`; | |
| } | |
| } | |
| // Direct tuple comparison with cursor values (no subquery needed) | |
| return sql`(${orderColumn}, ${domains.id}) ${sql.raw(op)} (${cursorValue}, ${cursorId})`; | |
| } |
🤖 Prompt for AI Agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts` around lines 410 - 438, The
tuple comparison in cursorFilter will break when cursorValue is null because
PostgreSQL tuple comparisons with NULL produce unknown; modify cursorFilter to
handle cursorValue === null explicitly: call getOrderColumn as now, compute
useGreaterThan/op, then branch — if cursorValue === null produce SQL that
compares nullness and id instead of a tuple (e.g., for "after" when ordering
ASC: rows with orderColumn IS NOT NULL should be considered greater than NULL,
and for rows where orderColumn IS NULL compare only the id), otherwise keep the
existing tuple comparison; update generated SQL to combine IS NULL / IS NOT NULL
checks with the id comparison (reference cursorFilter, getOrderColumn,
domains.id, DomainsOrderBy) so NULL cursor values are handled deterministically.
| export const DomainCursor = { | ||
| encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"), | ||
| decode: (cursor: string) => | ||
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor, |
There was a problem hiding this comment.
bigint values are not JSON-serializable and will fail at runtime.
JSON.stringify cannot serialize bigint values - it throws a TypeError. Since DomainOrderValue can be bigint (for REGISTRATION_TIMESTAMP and REGISTRATION_EXPIRY), cursor encoding will fail when ordering by these fields.
🐛 Proposed fix using a custom replacer/reviver
export const DomainCursor = {
- encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"),
- decode: (cursor: string) =>
- JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor,
+ encode: (cursor: DomainCursor) =>
+ Buffer.from(
+ JSON.stringify(cursor, (_, v) => (typeof v === "bigint" ? v.toString() + "n" : v)),
+ "utf8",
+ ).toString("base64"),
+ decode: (cursor: string) =>
+ JSON.parse(Buffer.from(cursor, "base64").toString("utf8"), (_, v) =>
+ typeof v === "string" && /^\d+n$/.test(v) ? BigInt(v.slice(0, -1)) : v,
+ ) as DomainCursor,
};📝 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.
| export const DomainCursor = { | |
| encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"), | |
| decode: (cursor: string) => | |
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor, | |
| export const DomainCursor = { | |
| encode: (cursor: DomainCursor) => | |
| Buffer.from( | |
| JSON.stringify(cursor, (_, v) => (typeof v === "bigint" ? v.toString() + "n" : v)), | |
| "utf8", | |
| ).toString("base64"), | |
| decode: (cursor: string) => | |
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8"), (_, v) => | |
| typeof v === "string" && /^\d+n$/.test(v) ? BigInt(v.slice(0, -1)) : v, | |
| ) as DomainCursor, | |
| }; |
🤖 Prompt for AI Agents
In `@apps/ensapi/src/graphql-api/schema/domain.ts` around lines 375 - 378,
DomainCursor.encode/decode currently use JSON.stringify/parse which throws on
bigint values (DomainOrderValue can be bigint for REGISTRATION_TIMESTAMP and
REGISTRATION_EXPIRY); update DomainCursor.encode and DomainCursor.decode to
serialize bigints safely by using a custom JSON replacer that converts bigint to
a string with a clear marker (e.g. prefix or wrapper) and a matching reviver in
decode that detects that marker and reconstructs the bigint, or alternately
explicitly convert those known fields (REGISTRATION_TIMESTAMP /
REGISTRATION_EXPIRY) to string before encoding and back to bigint after
decoding; ensure the changes touch the DomainCursor.encode and
DomainCursor.decode functions and preserve type assertions to DomainCursor after
reviving.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { AccountRegistryPermissionsRef } from "@/graphql-api/schema/account-registries-permissions"; | ||
| import { AccountResolverPermissionsRef } from "@/graphql-api/schema/account-resolver-permissions"; | ||
| import { DEFAULT_CONNECTION_ARGS } from "@/graphql-api/schema/constants"; | ||
| import { cursors } from "@/graphql-api/schema/cursors"; |
There was a problem hiding this comment.
The cursors import is no longer used in this file after the refactoring to use DomainCursor. This unused import should be removed.
| import { cursors } from "@/graphql-api/schema/cursors"; |
| const op = useGreaterThan ? ">" : "<"; | ||
|
|
||
| // Direct tuple comparison with cursor values (no subquery needed) | ||
| return sql`(${orderColumn}, ${domains.id}) ${sql.raw(op)} (${cursorValue}, ${cursorId})`; |
There was a problem hiding this comment.
The tuple comparison does not account for the NULLS LAST ordering specified in the orderFindDomains function. When the order column uses NULLS LAST, PostgreSQL places NULL values at the end of the result set. However, tuple comparison (orderColumn, id) > (NULL, cursorId) follows standard SQL NULL comparison rules, which may not align with NULLS LAST semantics.
This mismatch can cause incorrect pagination when cursors point to domains with NULL values in the order column. For example, when paginating forward (after cursor) with a NULL cursor value, the tuple comparison may include or exclude records incorrectly.
Consider handling NULL cursor values explicitly, or restructuring the comparison to account for the NULLS LAST behavior.
| encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"), | ||
| decode: (cursor: string) => | ||
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor, |
There was a problem hiding this comment.
The JSON.stringify call will throw a TypeError when cursor.value is a bigint (which happens for REGISTRATION_TIMESTAMP and REGISTRATION_EXPIRY orderings). JavaScript's JSON.stringify does not support bigint values and will throw "TypeError: Do not know how to serialize a BigInt".
Consider converting bigint values to strings before stringifying, or adding a custom replacer function to handle bigint serialization.
| encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"), | |
| decode: (cursor: string) => | |
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor, | |
| encode: (cursor: DomainCursor) => | |
| Buffer.from( | |
| JSON.stringify( | |
| cursor, | |
| (_key, value) => (typeof value === "bigint" ? value.toString() : value), | |
| ), | |
| "utf8", | |
| ).toString("base64"), | |
| decode: (cursor: string) => { | |
| const parsed = JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor; | |
| if ( | |
| parsed.value !== null && | |
| parsed.value !== undefined && | |
| (parsed.by === "REGISTRATION_TIMESTAMP" || parsed.by === "REGISTRATION_EXPIRY") | |
| ) { | |
| parsed.value = BigInt(parsed.value as unknown as string); | |
| } | |
| return parsed; | |
| }, |
| encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"), | ||
| decode: (cursor: string) => | ||
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor, |
There was a problem hiding this comment.
The JSON.parse call will not restore bigint values correctly - they will be deserialized as numbers, which can cause precision loss for large timestamp values. When paired with the encoding issue in line 381, this will cause a mismatch between the type of value that was intended (bigint) and what's actually decoded (number).
Consider implementing custom deserialization logic that converts timestamp values back to bigints based on the by field.
| encode: (cursor: DomainCursor) => Buffer.from(JSON.stringify(cursor), "utf8").toString("base64"), | |
| decode: (cursor: string) => | |
| JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor, | |
| encode: (cursor: DomainCursor) => { | |
| // Normalize bigint values to strings before JSON serialization | |
| const serializableCursor: DomainCursor = { | |
| ...cursor, | |
| value: typeof cursor.value === "bigint" ? cursor.value.toString() : cursor.value, | |
| }; | |
| return Buffer.from(JSON.stringify(serializableCursor), "utf8").toString("base64"); | |
| }, | |
| decode: (cursor: string): DomainCursor => { | |
| const parsed = JSON.parse(Buffer.from(cursor, "base64").toString("utf8")) as DomainCursor; | |
| // For non-NAME orderings (timestamps, etc.), convert value back to bigint | |
| if (parsed.by !== "NAME" && parsed.value !== null && parsed.value !== undefined) { | |
| if (typeof parsed.value === "string" || typeof parsed.value === "number") { | |
| return { | |
| ...parsed, | |
| value: BigInt(parsed.value), | |
| }; | |
| } | |
| } | |
| return parsed; | |
| }, |
| const useGreaterThan = (direction === "after") !== effectiveDesc; | ||
| const op = useGreaterThan ? ">" : "<"; | ||
|
|
||
| // Direct tuple comparison with cursor values (no subquery needed) |
There was a problem hiding this comment.
Tuple comparison with NULL or undefined cursor values may produce incorrect pagination results. When cursorValue is NULL/undefined (which can happen for domains without registration data), the SQL tuple comparison (orderColumn, id) > (NULL, cursorId) may not work as expected. PostgreSQL's tuple comparison treats NULL specially, and this could cause domains with NULL values to be incorrectly included or excluded from result sets.
Consider adding explicit NULL handling logic before the tuple comparison, or converting NULL/undefined to a sentinel value that works correctly with tuple comparisons.
| // Direct tuple comparison with cursor values (no subquery needed) | |
| // When the cursor's order value is NULL/undefined (for example, domains without | |
| // registration data), avoid tuple comparison with NULL because PostgreSQL | |
| // treats any comparison with NULL as UNKNOWN (not TRUE). | |
| // | |
| // In this case, we: | |
| // - Stay within the NULL partition of the ordered column, and | |
| // - Use the domain id alone as the keyset pivot with the same direction. | |
| if (cursorValue == null) { | |
| const idComparison = useGreaterThan | |
| ? sql`${domains.id} > ${cursorId}` | |
| : sql`${domains.id} < ${cursorId}`; | |
| return and( | |
| // Ensure we only consider rows where the order column is NULL | |
| sql`${orderColumn} IS NULL`, | |
| idComparison, | |
| ); | |
| } | |
| // Direct tuple comparison with non-NULL cursor values (no subquery needed) |
| export type DomainsOrderByValue = typeof DomainsOrderBy.$inferType; | ||
|
|
||
| export const DomainsOrderInput = builder.inputType("DomainsOrderInput", { | ||
| description: "Ordering options for domains query", |
There was a problem hiding this comment.
The by field in DomainsOrderInput is marked as required, which means users must specify which field to order by even if they only want to change the direction. This makes it impossible to use only the default ordering field with a custom direction (e.g., to get NAME DESC without explicitly specifying by: NAME).
Consider making the by field optional and using the DOMAINS_DEFAULT_ORDER_BY when not provided, or document that both fields must be specified if order is provided.
| description: "Ordering options for domains query", | |
| description: | |
| "Ordering options for domains query. When providing a custom order, both `by` and `dir` must be specified. If no order is provided, the default is NAME ASC.", |
| import { AccountRef } from "@/graphql-api/schema/account"; | ||
| import { AccountIdInput } from "@/graphql-api/schema/account-id"; | ||
| import { DEFAULT_CONNECTION_ARGS } from "@/graphql-api/schema/constants"; | ||
| import { cursors } from "@/graphql-api/schema/cursors"; |
There was a problem hiding this comment.
The cursors import is no longer used in this file after the refactoring to use DomainCursor. This unused import should be removed.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Drizzle 0.41 doesn't support gt/lt with tuple arrays, so we use raw SQL for the cursor filter tuple comparison. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -3,18 +3,32 @@ import { and, asc, desc, eq, gt, lt } from "drizzle-orm"; | |||
| import type { Address } from "viem"; | |||
There was a problem hiding this comment.
The imports asc, desc, gt, and lt from drizzle-orm are no longer used after the refactoring to use the new ordering and pagination functions. These should be removed from the import statement to keep the imports clean.
| resolve: (parent, args, context) => { | ||
| const orderBy = args.order?.by ?? DOMAINS_DEFAULT_ORDER_BY; | ||
| const orderDir = args.order?.dir ?? DOMAINS_DEFAULT_ORDER_DIR; | ||
|
|
||
| return resolveCursorConnection( | ||
| { | ||
| ...DEFAULT_CONNECTION_ARGS, | ||
| args, | ||
| toCursor: (domain: DomainWithOrderValue) => | ||
| DomainCursor.encode({ | ||
| id: domain.id, | ||
| by: orderBy, | ||
| value: domain.__orderValue, | ||
| }), | ||
| }, | ||
| async ({ before, after, limit, inverted }: ResolveCursorConnectionArgs) => { | ||
| const effectiveDesc = isEffectiveDesc(orderDir, inverted); | ||
|
|
||
| // construct query for relevant domains | ||
| const domains = findDomains({ ...args.where, owner: parent.id }); | ||
|
|
||
| // execute with pagination constraints | ||
| // build order clauses | ||
| const orderClauses = orderFindDomains(domains, orderBy, orderDir, inverted); | ||
|
|
||
| // decode cursors for keyset pagination | ||
| const beforeCursor = before ? DomainCursor.decode(before) : undefined; | ||
| const afterCursor = after ? DomainCursor.decode(after) : undefined; | ||
|
|
||
| // execute with pagination constraints using tuple comparison | ||
| const results = await db | ||
| .with(domains) | ||
| .select() | ||
| .from(domains) | ||
| .where( | ||
| and( | ||
| before ? lt(domains.id, cursors.decode<DomainId>(before)) : undefined, | ||
| after ? gt(domains.id, cursors.decode<DomainId>(after)) : undefined, | ||
| beforeCursor | ||
| ? cursorFilter(domains, beforeCursor, orderBy, "before", effectiveDesc) | ||
| : undefined, | ||
| afterCursor | ||
| ? cursorFilter(domains, afterCursor, orderBy, "after", effectiveDesc) | ||
| : undefined, | ||
| ), | ||
| ) | ||
| .orderBy(inverted ? desc(domains.id) : asc(domains.id)) | ||
| .orderBy(...orderClauses) | ||
| .limit(limit); | ||
|
|
||
| // provide full Domain entities via dataloader | ||
| return rejectAnyErrors( | ||
| // Map CTE results by id for order value lookup | ||
| const orderValueById = new Map( | ||
| results.map((r) => [r.id, getOrderValueFromResult(r, orderBy)]), | ||
| ); | ||
|
|
||
| // Load full Domain entities via dataloader | ||
| const loadedDomains = await rejectAnyErrors( | ||
| DomainInterfaceRef.getDataloader(context).loadMany( | ||
| results.map((result) => result.id), | ||
| ), | ||
| ); | ||
|
|
||
| // Attach order values for cursor encoding | ||
| return loadedDomains.map((domain) => ({ | ||
| ...domain, | ||
| __orderValue: orderValueById.get(domain.id), | ||
| })); | ||
| }, | ||
| ), | ||
| ); | ||
| }, |
There was a problem hiding this comment.
There is significant code duplication between the domains resolver in query.ts (lines 65-130) and the domains resolver in account.ts (lines 83-148). The only difference is on line 102 in account.ts where parent.id is passed as the owner. Consider extracting this logic into a shared helper function to improve maintainability and reduce duplication.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts`:
- Around line 393-405: getOrderColumn currently maps DomainsOrderBy values to
columns directly and would return undefined for unknown/new enum values causing
invalid SQL; replace the object lookup with a switch on orderBy in the
getOrderColumn function (referencing getOrderColumn, DomainsOrderBy, and the
domains parameter from findDomains) and add an exhaustive never branch that
throws or asserts on unhandled cases so any new enum value fails at compile time
/ runtime instead of returning undefined.
In `@apps/ensapi/src/graphql-api/lib/resolve-find-domains.ts`:
- Around line 54-63: The cursor currently encodes only { id, by, value } via
DomainCursor.encode in resolveCursorConnection, which allows a client to change
the pagination direction (dir) between requests and get inconsistent results;
update the cursor encoding to include the pagination direction (e.g., add dir)
when calling DomainCursor.encode and update the DomainCursor type/schema
accordingly, then update cursor decoding/validation (the place that currently
checks cursor.by === queryOrderBy) to also assert decodedCursor.dir === queryDir
before using it in cursorFilter/tuple comparisons so pagination direction and
order are always matched.
| /** | ||
| * Get the order column for a given DomainsOrderBy value. | ||
| */ | ||
| function getOrderColumn( | ||
| domains: ReturnType<typeof findDomains>, | ||
| orderBy: typeof DomainsOrderBy.$inferType, | ||
| ) { | ||
| return { | ||
| NAME: domains.leafLabelValue, | ||
| REGISTRATION_TIMESTAMP: domains.registrationStart, | ||
| REGISTRATION_EXPIRY: domains.registrationExpiry, | ||
| }[orderBy]; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard against unknown orderBy values in getOrderColumn.
If a new enum value is added, this will return undefined and generate invalid SQL. A switch with a never guard avoids that.
♻️ Proposed fix
function getOrderColumn(
domains: ReturnType<typeof findDomains>,
orderBy: typeof DomainsOrderBy.$inferType,
) {
- return {
- NAME: domains.leafLabelValue,
- REGISTRATION_TIMESTAMP: domains.registrationStart,
- REGISTRATION_EXPIRY: domains.registrationExpiry,
- }[orderBy];
+ switch (orderBy) {
+ case "NAME":
+ return domains.leafLabelValue;
+ case "REGISTRATION_TIMESTAMP":
+ return domains.registrationStart;
+ case "REGISTRATION_EXPIRY":
+ return domains.registrationExpiry;
+ default: {
+ const _exhaustive: never = orderBy;
+ throw new Error(`Unhandled orderBy: ${_exhaustive}`);
+ }
+ }
}🤖 Prompt for AI Agents
In `@apps/ensapi/src/graphql-api/lib/find-domains.ts` around lines 393 - 405,
getOrderColumn currently maps DomainsOrderBy values to columns directly and
would return undefined for unknown/new enum values causing invalid SQL; replace
the object lookup with a switch on orderBy in the getOrderColumn function
(referencing getOrderColumn, DomainsOrderBy, and the domains parameter from
findDomains) and add an exhaustive never branch that throws or asserts on
unhandled cases so any new enum value fails at compile time / runtime instead of
returning undefined.
| return resolveCursorConnection( | ||
| { | ||
| ...DEFAULT_CONNECTION_ARGS, | ||
| args, | ||
| toCursor: (domain: DomainWithOrderValue) => | ||
| DomainCursor.encode({ | ||
| id: domain.id, | ||
| by: orderBy, | ||
| value: domain.__orderValue, | ||
| }), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "resolve-find-domains.ts" | head -20Repository: namehash/ensnode
Length of output: 117
🏁 Script executed:
git ls-files "apps/ensapi/src/graphql-api/lib/" | head -30Repository: namehash/ensnode
Length of output: 486
🏁 Script executed:
wc -l apps/ensapi/src/graphql-api/lib/resolve-find-domains.tsRepository: namehash/ensnode
Length of output: 119
🏁 Script executed:
cat -n apps/ensapi/src/graphql-api/lib/resolve-find-domains.tsRepository: namehash/ensnode
Length of output: 4417
🏁 Script executed:
find . -type f -name "domain.ts" -path "*/schema/*"Repository: namehash/ensnode
Length of output: 106
🏁 Script executed:
rg "export.*DomainCursor" -A 10Repository: namehash/ensnode
Length of output: 1247
🏁 Script executed:
rg "function cursorFilter|const cursorFilter|export.*cursorFilter" -A 20 apps/ensapi/src/graphql-api/lib/find-domains.tsRepository: namehash/ensnode
Length of output: 807
🏁 Script executed:
rg "export function cursorFilter" -A 50 apps/ensapi/src/graphql-api/lib/find-domains.tsRepository: namehash/ensnode
Length of output: 1939
🏁 Script executed:
rg "const OrderDirection|export.*OrderDirection" -B 2 -A 10 apps/ensapi/src/graphql-api/schema/Repository: namehash/ensnode
Length of output: 1737
Include direction in the cursor to prevent mismatched pagination.
The cursor only encodes by; if a client changes dir between requests, cursorFilter will accept the cursor and return inconsistent results because the tuple comparison direction will be inverted. The current validation only checks cursor.by === queryOrderBy but does not validate the direction context.
♻️ Proposed update (plus schema/type changes)
toCursor: (domain: DomainWithOrderValue) =>
DomainCursor.encode({
id: domain.id,
by: orderBy,
+ dir: orderDir,
value: domain.__orderValue,
}),🤖 Prompt for AI Agents
In `@apps/ensapi/src/graphql-api/lib/resolve-find-domains.ts` around lines 54 -
63, The cursor currently encodes only { id, by, value } via DomainCursor.encode
in resolveCursorConnection, which allows a client to change the pagination
direction (dir) between requests and get inconsistent results; update the cursor
encoding to include the pagination direction (e.g., add dir) when calling
DomainCursor.encode and update the DomainCursor type/schema accordingly, then
update cursor decoding/validation (the place that currently checks cursor.by ===
queryOrderBy) to also assert decodedCursor.dir === queryDir before using it in
cursorFilter/tuple comparisons so pagination direction and order are always
matched.
closes #1564