Conversation
decompose monolithic findDomains into composable filter layers: - domainsBase(): universal base set with derived v2 parentId and sortableLabel - selectBase(): helper for consistent column selection across layers - filterByOwner/filterByParent/filterByRegistry/filterByCanonical/filterByName - withOrderingMetadata: final CTE for cursor pagination each GraphQL entry point composes its own pipeline: - Query.domains: domainsBase → filterByName → filterByCanonical → withOrderingMetadata - Account.domains: domainsBase → filterByOwner → filterByName → filterByCanonical → withOrderingMetadata - Registry.domains: domainsBase → filterByRegistry → filterByName → withOrderingMetadata - Domain.subdomains: domainsBase → filterByParent → filterByName → withOrderingMetadata Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rename output column from `registryId` to `id` in getCanonicalRegistriesCTE to prevent column name clash when LEFT JOINed with base domain set's `registryId` column. Drizzle doesn't qualify subquery column references in JOINs, causing PostgreSQL "column reference is ambiguous" errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 388c9bd 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
|
📝 WalkthroughWalkthroughThis pull request refactors the domain-finding pipeline from a monolithic function into a composable, layered architecture. The new system builds a base domain set combining v1 and v2 sources, applies sequential filters (canonical, name, owner, registry, parent), enriches with ordering metadata, and integrates across GraphQL schema endpoints (Account, Domain, Query, Registry). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
Greptile SummaryThis PR successfully refactors Key improvements:
Breaking changes:
Notable design decisions:
The layer contracts are clear and maintainable - each layer accepts a Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Start[Entry Point: Query.domains / Account.domains / Domain.subdomains / Registry.domains]
Base[domainsBase: Universal v1+v2 union with derived parentId]
FilterName[filterByName: Path traversal + LIKE matching]
FilterOwner[filterByOwner: owner address filter]
FilterParent[filterByParent: parent domain filter]
FilterRegistry[filterByRegistry: registry ID filter]
FilterCanonical[filterByCanonical: LEFT JOIN canonical registries CTE]
WithMetadata[withOrderingMetadata: JOIN registration data]
Resolver[resolveFindDomains: cursor pagination + dataloader]
Result[Return: Domain connection]
Start --> Base
Base --> FilterName
Base --> FilterOwner
Base --> FilterParent
Base --> FilterRegistry
FilterName --> FilterCanonical
FilterOwner --> FilterName
FilterOwner --> FilterCanonical
FilterParent --> FilterName
FilterRegistry --> FilterName
FilterName --> WithMetadata
FilterCanonical --> WithMetadata
WithMetadata --> Resolver
Resolver --> Result
style Base fill:#e1f5ff
style WithMetadata fill:#ffe1f5
style Resolver fill:#f5ffe1
Last reviewed commit: 11d9564 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the monolithic findDomains query function into a composable layer architecture, enabling better code reuse and more flexible domain querying across multiple GraphQL entry points.
Changes:
- Decomposed
findDomainsinto composable query layers:domainsBase()(universal v1+v2 union), filter layers (canonical, name, owner, parent, registry), andwithOrderingMetadata() - Added new
Account.domainsconnection field for querying domains by owner with optional name/canonical filters - Enhanced
Domain.subdomainsandRegistry.domainswith name filtering and ordering support - Breaking changes to
Query.domains:where.nameis now required,where.ownerremoved (moved toAccount.domains)
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
apps/ensapi/src/graphql-api/lib/find-domains/layers/base-domain-set.ts |
Universal base domain set with v1+v2 union and derived v2 parentId via canonical registry traversal |
apps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-canonical.ts |
Filters domains to only canonical (v1 domains + v2 domains in canonical registries) |
apps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-name.ts |
Applies path traversal and partial LIKE matching for name-based filtering |
apps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-owner.ts |
Filters domains by owner address |
apps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-parent.ts |
Filters domains to children of a specific parent (uniform v1/v2 handling) |
apps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-registry.ts |
Filters domains to those belonging to a specific registry |
apps/ensapi/src/graphql-api/lib/find-domains/layers/with-ordering-metadata.ts |
Enriches domain set with registration metadata for ordering support |
apps/ensapi/src/graphql-api/lib/find-domains/layers/index.ts |
Exports all layer functions for composable use |
apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts |
Removed monolithic findDomains function, retained utility functions for ordering and cursor filtering |
apps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver.ts |
Updated to accept pre-built domains CTE instead of where args |
apps/ensapi/src/graphql-api/lib/find-domains/types.ts |
Removed FindDomainsWhereArg, updated comments to reference new architecture |
apps/ensapi/src/graphql-api/lib/canonical-registries-cte.ts |
Renamed output column from registryId to id to avoid drizzle column ambiguity |
apps/ensapi/src/graphql-api/schema/account.ts |
Added Account.domains connection field using layer composition |
apps/ensapi/src/graphql-api/schema/domain.ts |
Enhanced Domain.subdomains, added per-field input types (DomainsWhereInput, AccountDomainsWhereInput, RegistryDomainsWhereInput, SubdomainsWhereInput) |
apps/ensapi/src/graphql-api/schema/registry.ts |
Enhanced Registry.domains with filtering/ordering, changed return type from ENSv2Domain to DomainInterface |
apps/ensapi/src/graphql-api/schema/query.ts |
Updated Query.domains to use layer composition, where.name now required |
.changeset/refactor-find-domains-layers.md |
Documents new Account.domains field and enhancements to Domain.subdomains and Registry.domains |
Comments suppressed due to low confidence (2)
.changeset/refactor-find-domains-layers.md:17
- The changeset should document that the return type of
Registry.domainshas changed fromENSv2DomaintoDomainInterface. While in practice this field will only return v2 domains (since only v2 domains have a non-null registryId), this is a breaking change for type-sensitive GraphQL clients that may need to update their queries to handle the more general interface type.
**`Registry.domains`** (enhanced) — paginated connection of domains in this registry, now with filtering and ordering.
- `where: { name?: String }` — optional partial Interpreted Name filter
- `order: { by: NAME | REGISTRATION_TIMESTAMP | REGISTRATION_EXPIRY, dir: ASC | DESC }` — ordering
.changeset/refactor-find-domains-layers.md:21
- The changeset documents that
where.nameis now required forQuery.domains, but does not document thatwhere.ownerhas been removed. This is a breaking change that should be explicitly documented. Consider adding a note like: "Breaking:where.ownerhas been removed fromQuery.domains- use the newAccount.domainsfield instead to query domains by owner."
**`Query.domains`** (updated) — `where.name` is now required. Added optional `where.canonical` filter (defaults to false).
- `where: { name: String!, canonical?: Boolean }` — required partial Interpreted Name, optional canonical filter
- `order: { by: NAME | REGISTRATION_TIMESTAMP | REGISTRATION_EXPIRY, dir: ASC | DESC }` — ordering
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
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/lib/find-domains/find-domains.ts (1)
104-109: 🧹 Nitpick | 🔵 Trivial
orderFindDomainsis the only exported function without a doc comment.Every other exported function in this file (
cursorFilter,isEffectiveDesc) and the privategetOrderColumncarry JSDoc blocks. A brief comment here would restore consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts` around lines 104 - 109, The exported function orderFindDomains lacks a JSDoc comment like the other exports; add a brief JSDoc block above orderFindDomains describing its purpose, parameters and return type (e.g., that it builds SQL ordering clauses for DomainsCTE using orderBy: DomainsOrderBy, orderDir: OrderDirection and inverted flag) and reference any relevant helpers such as getOrderColumn, cursorFilter and isEffectiveDesc to keep documentation consistent with the rest of the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Line 36: Remove the redundant JSDoc `@returns` tag that restates the TypeScript
return type (: SQL) for the function that builds the cursor filter SQL
expression; locate the JSDoc immediately above the function returning SQL (the
cursor-filter builder) and delete the line "@returns SQL expression for the
cursor filter", leaving the rest of the comment intact.
In `@apps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-name.ts`:
- Around line 37-56: The schema allows empty string names, causing
filterByName(base, name) to receive "" and skip the LIKE filter; update the
GraphQL input definitions by adding minLength: 1 to the name fields on the four
WhereInput types so empty strings are rejected: add minLength: 1 to the name
field of DomainsWhereInput, AccountDomainsWhereInput, RegistryDomainsWhereInput,
and SubdomainsWhereInput (the GraphQL schema input definitions that declare the
name property) so queries always provide at least one character and filterByName
will receive a non-empty string.
---
Outside diff comments:
In `@apps/ensapi/src/graphql-api/lib/find-domains/find-domains.ts`:
- Around line 104-109: The exported function orderFindDomains lacks a JSDoc
comment like the other exports; add a brief JSDoc block above orderFindDomains
describing its purpose, parameters and return type (e.g., that it builds SQL
ordering clauses for DomainsCTE using orderBy: DomainsOrderBy, orderDir:
OrderDirection and inverted flag) and reference any relevant helpers such as
getOrderColumn, cursorFilter and isEffectiveDesc to keep documentation
consistent with the rest of the file.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (17)
.changeset/refactor-find-domains-layers.mdapps/ensapi/src/graphql-api/lib/canonical-registries-cte.tsapps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver.tsapps/ensapi/src/graphql-api/lib/find-domains/find-domains.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/base-domain-set.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-canonical.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-name.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-owner.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-parent.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/filter-by-registry.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/index.tsapps/ensapi/src/graphql-api/lib/find-domains/layers/with-ordering-metadata.tsapps/ensapi/src/graphql-api/lib/find-domains/types.tsapps/ensapi/src/graphql-api/schema/account.tsapps/ensapi/src/graphql-api/schema/domain.tsapps/ensapi/src/graphql-api/schema/query.tsapps/ensapi/src/graphql-api/schema/registry.ts
apps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver-helpers.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.changeset/refactor-find-domains-layers.md:3
- The changeset specifies this as a
patchrelease, but the PR introduces breaking changes toQuery.domains(makingwhere.namerequired and removingwhere.owner). This should be at least aminorversion bump, or possiblymajordepending on the versioning policy of this package.
Additionally, the changeset doesn't explicitly document the removal of where.owner from Query.domains and the recommended migration path (use Account.domains instead). Consider adding this breaking change to the changeset description.
---
"ensapi": patch
---
💡 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 19 out of 19 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
apps/ensapi/src/graphql-api/lib/find-domains/find-domains-resolver-helpers.test.ts:5
- This mock references a file path that no longer exists. The
v1DomainsByLabelHashPathandv2DomainsByLabelHashPathfunctions were moved intolayers/filter-by-name.tsas part of this refactor. Since this test file only importsisEffectiveDescfromfind-domains-resolver-helpersand doesn't use anything from the previously separate labelhash-path file, this mock can be safely removed.
.changeset/refactor-find-domains-layers.md:18 - The changeset doesn't document that
Registry.domainshas a breaking return type change fromENSv2DomaintoDomainInterface. While in practice this may only return v2 domains (since only v2 domains have a registryId), the GraphQL schema type has changed, which could affect clients performing type checks or fragment matching. This should be documented in the changeset as a breaking change.
**`Registry.domains`** (enhanced) — paginated connection of domains in this registry, now with filtering and ordering.
- `where: { name?: String }` — optional partial Interpreted Name filter
- `order: { by: NAME | REGISTRATION_TIMESTAMP | REGISTRATION_EXPIRY, dir: ASC | DESC }` — ordering
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Beautiful refactoring. Definitely helps make this logic easier to wrap your head around.
findDomainsinto composable query layersAccount.domainsQuery.domainsDomain.subdomainsandRegistry.domainsReviewer Focus (Read This First)
find-domains/layers/— is the shape/contract between layers clear and maintainable?domainsBase()universal base set, particularly the v2 parentId derivation via canonical registry traversal (registryCanonicalDomain→ reverse pointer verification)Query.domains:where.nameis now required,where.ownerwas removed (moved toAccount.domains)Problem & Motivation
findDomainswas a monolithic function that handled path traversal, owner/canonical/partial filtering, and ordering metadata in one giant CTEDomain.subdomainsandRegistry.domainsused simple cursor pagination with no name filtering or ordering supportAccount.domainsfield existed — owner filtering was baked intoQuery.domainsviawhere.ownerWhat Changed (Concrete)
findDomainsinto composable query layers infind-domains/layers/:domainsBase()— universal v1+v2 union with v2 parentId derived viaregistryCanonicalDomaintraversal, sortableLabel, registryIdfilterByName(base, name?)— path traversal + partial LIKE matchingfilterByCanonical(base)— canonical registry filtering via recursive CTEfilterByOwner(base, owner)— owner address filteringfilterByParent(base, parentId)— parent domain filtering (uniform v1/v2)filterByRegistry(base, registryId)— registry ID filteringwithOrderingMetadata(base)— join registration timestamps/expiry for orderingAccount.domainsconnection field withwhere: { name?: String, canonical?: Boolean }andorderargsDomain.subdomainswithwhere: { name?: String }andorderargs (was simple id-cursor pagination)Registry.domainswithwhere: { name?: String }andorderargs; return type changed fromENSv2DomaintoDomainInterfaceQuery.domains:where.nameis now required,where.ownerremoved (useAccount.domainsinstead)DomainsWhereInput,AccountDomainsWhereInput,RegistryDomainsWhereInput,SubdomainsWhereInputresolveFindDomainsnow accepts a pre-builtDomainsCTEinstead of callingfindDomainsinternallyregistryId→idto avoid drizzle column name ambiguity in LEFT JOINsDesign & Planning
planned iteratively via claude code plan mode; each layer was validated against generated SQL output
the key design decision was a single universal base set (
domainsBase()) rather than specialized base sets per entry point — simpler composition at the cost of a larger base union, but postgres predicate pushdown into union branches shouldhandle this
v2 parentId is derived in the base set via canonical registry traversal: LEFT JOIN
registryCanonicalDomainto find the declared canonical domain for the child's registry, then LEFT JOINv2ParentDomainto verify the reverse pointer(
parent.subregistryId = child.registryId). this is a single-hop version of whatgetV2CanonicalPathdoes recursively — eliminates v1/v2 branching infilterByParentsortableLabelpattern: base set provides the domain's own label;filterByNameoverrides it with the head domain's label when a concrete path is present (leafId ≠ headId in path traversal)Planning artifacts:
.claude/plans/humble-yawning-swing.mdReviewed / approved by: n/a
Self-Review
found and fixed drizzle column name ambiguity:
registryIdexisted in both the base set and canonical registries CTE, causing "column reference 'registryId' is ambiguous" at runtime. fixed by renaming the CTE output column toid.simplified filterByCanonical from IN-subquery approach back to LEFT JOIN after identifying the column rename as the correct fix
Bugs caught: ambiguous column reference in filterByCanonical LEFT JOIN
Logic simplified: filterByParent — no v1/v2 branching needed thanks to derived parentId in base set
Naming / terminology improved:
FindDomainsWhereArgremoved, replaced by per-field input typesDead or unnecessary code removed: ~200 lines removed from
find-domains.tsCross-Codebase Alignment
findDomains,resolveFindDomains,FindDomainsWhereArg,DomainsWhereInput,subdomains,Registry.domainsfind-domains-by-labelhash-path.ts(reused by filterByName),canonical-registries-cte.ts(reused by filterByCanonical, only column rename),domain-cursor.ts,get-canonical-path.tsENSv1Domain.childrenleft as-is — simple id-cursor connection, doesn't need the composable layer treatmentDownstream & Consumer Impact
breaking:
Query.domainswhere.nameis now required (was optional).where.ownerremoved — useAccount.domainsinstead.breaking:
Registry.domainsreturn type changed fromENSv2DomaintoDomainInterface(now includes v1 domains if registry filter somehow matches, though in practice only v2 domains have non-null registryId)new:
Account.domainsconnection field for querying domains by owner with optional name/canonical filters and orderingenhanced:
Domain.subdomainsandRegistry.domainsnow support name filtering and ordering (NAME, REGISTRATION_TIMESTAMP, REGISTRATION_EXPIRY)Public APIs affected:
Query.domains,Account.domains(new),Domain.subdomains,Registry.domainsDocs updated: changeset included
Naming decisions worth calling out: separate input types per field (
DomainsWhereInput,AccountDomainsWhereInput, etc.) rather than one shared type — keeps each field's contract preciseTesting Evidence
typecheck, lint, and test all pass
manually tested all four entry points against a running instance with indexed data
verified canonical filtering works independently of name filtering on
Account.domainsTesting performed:
pnpm typecheck,pnpm lint,pnpm test, manual GraphQL queriesKnown gaps: no automated integration tests for the new entry points or layer composition
What reviewers have to reason about manually (and why): correctness of generated SQL for each pipeline composition — drizzle's subquery aliasing behavior is non-obvious, particularly around column name qualification in JOINs
Scope Reductions
ENSv1Domain.childrenleft as simple id-cursor connection (no name/ordering support)Risk Analysis
drizzle column name ambiguity is a known footgun — any future layer that JOINs a table with columns named
domainId,ownerId,registryId, etc. could hit the same issue. mitigated by using distinct column names in CTEs.the universal base set scans all v1+v2 domains before filtering. postgres predicate pushdown should optimize this, but if it doesn't, query performance could regress for large datasets.
Risk areas: query performance on large datasets, drizzle column aliasing edge cases
Mitigations or rollback options: revert to previous monolithic
findDomainsif needed; SQL debug logging in resolver aids diagnosisNamed owner if this causes problems: @shrugs
Pre-Review Checklist (Blocking)