Skip to content

ENSv2 GraphQL API Devnet Integration Test Scaffolding#1693

Open
shrugs wants to merge 14 commits intomainfrom
feat/graphql-api-integration-test
Open

ENSv2 GraphQL API Devnet Integration Test Scaffolding#1693
shrugs wants to merge 14 commits intomainfrom
feat/graphql-api-integration-test

Conversation

@shrugs
Copy link
Collaborator

@shrugs shrugs commented Feb 26, 2026

Reviewer Focus (Read This First)

  • vitest config changes across all 12 projects — each now excludes *.integration.test.ts via configDefaults.exclude spread. this is necessary because vitest workspace mode doesn't propagate root-level excludes to project configs.
  • the test helpers (graphql-utils.ts, highlight.ts) — these are shared utilities for ensapi integration tests
  • the devnet name expectations in devnet-names.ts — these encode assumptions about the devnet state and canonical vs aliased names.

Problem & Motivation

  • ensapi's graphql api had zero integration test coverage. all validation was manual against a running instance.
  • as the schema evolves, we need automated verification against real indexed data.
  • next up is ENSv2 GraphQL Api Integration Smoke Tests #1622 for establishing more test cases

What Changed (Concrete)

  1. added vitest.integration.config.ts at root and apps/ensapi/ to define a separate integration test project
  2. updated all 12 project vitest.config.ts files to exclude **/*.integration.test.ts from unit test runs
  3. added pnpm test:integration script at root and in apps/ensapi/package.json
  4. created integration test infrastructure:
    • global-setup.ts — health check via introspection query before tests run
    • ensnode-graphql-api-client.ts — shared GraphQLClient instance + gql tag
    • graphql-utils.tsrequest() helper with pretty-printed request/response logging, flattenConnection() helper
    • highlight.ts — prismjs + chalk syntax highlighting for graphql and json output
    • devnet-names.ts — shared devnet name fixtures with canonical mappings
  5. added integration tests:
    • query.integration.test.tsQuery.root, Query.domains, Query.domain (parameterized over all devnet names)
    • domain.integration.test.tsDomain.subdomains for .eth
    • account.integration.test.tsAccount.domains for default and transferred owners
    • registry.integration.test.tsRegistry.domains for ETH registry
  6. added documentation comment to get-domain-by-interpreted-name.ts explaining forward traversal semantics, expired name handling, and alias behavior

Design & Planning

  • planning was lightweight — started from a plan doc, iterated on patterns (test helpers, assertion style, parameterization) during implementation

  • key decisions: use graphql-request for gql tag and typed client; use prismjs+chalk for terminal highlighting; avoiding too much generalization and keeping the integration tests tightly coupled to a running ensapi w/ an ensindexer that has indexed devnet

  • Planning artifacts: Claude Code plan mode

  • Reviewed / approved by: n/a


Self-Review

  • Bugs caught: root vitest exclude doesn't propagate to project configs in workspace mode — had to add per-project excludes and overriding exclude without spreading configDefaults.exclude caused node_modules test files to run

Downstream & Consumer Impact

  • no public api changes

  • no runtime behavior changes

  • the get-domain-by-interpreted-name.ts comment is documentation-only

  • Public APIs affected: none

  • Docs updated: added jsdoc to getDomainIdByInterpretedName

  • Naming decisions worth calling out: DEVNET_ETH_LABELS derives from DEVNET_NAMES by filtering canonical 2ld .eth names


Testing Evidence

  • tests are designed to run against a live devnet instance via pnpm test:integration

  • pnpm test at root confirms integration tests are excluded from unit test runs

  • global setup fails fast with actionable error if ensapi is unreachable

  • Testing performed: manual runs against local devnet

  • Known gaps: no CI integration yet; tests assume specific devnet state


Scope Reductions

  • Follow-ups:
    • CI pipeline integration for running integration tests against devnet
    • pagination / connection arg testing
  • Why they were deferred: this PR establishes the framework; additional coverage builds on it

Risk Analysis

  • Risk areas: vitest config changes touch every project — incorrect exclude patterns could hide or expose tests unexpectedly
  • Mitigations or rollback options: pnpm test output shows file count, easy to verify no integration tests leak into unit runs
  • Named owner if this causes problems: @shrugs

Pre-Review Checklist (Blocking)

  • I reviewed every line of this diff and understand it end-to-end
  • I'm prepared to defend this PR line-by-line in review
  • I'm comfortable being the on-call owner for this change
  • Relevant changesets are included (or explicitly not required)

shrugs and others added 6 commits February 25, 2026 15:17
…ration tests

Adds prismjs+chalk syntax highlighting for GraphQL/JSON output, a request
helper with pretty logging, and parameterized domain resolution tests
covering all devnet names.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract DEVNET_NAMES and DEVNET_ETH_LABELS into shared test util.
Add Domain.subdomains, Account.domains, and Registry.domains tests
verifying devnet state.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@shrugs shrugs requested a review from a team as a code owner February 26, 2026 20:32
Copilot AI review requested due to automatic review settings February 26, 2026 20:32
@vercel
Copy link
Contributor

vercel bot commented Feb 26, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
admin.ensnode.io Ready Ready Preview, Comment Feb 26, 2026 11:23pm
ensnode.io Ready Ready Preview, Comment Feb 26, 2026 11:23pm
ensrainbow.io Ready Ready Preview, Comment Feb 26, 2026 11:23pm

@changeset-bot
Copy link

changeset-bot bot commented Feb 26, 2026

⚠️ No Changeset found

Latest commit: ebb181c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 26, 2026

Warning

Rate limit exceeded

@shrugs has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 7 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 3e91b4a and ebb181c.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (5)
  • apps/ensapi/package.json
  • apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts
  • apps/ensapi/src/test/integration/global-setup.ts
  • apps/ensapi/src/test/integration/graphql-utils.ts
  • package.json
📝 Walkthrough

Walkthrough

This PR adds integration-test infrastructure and suites: new Vitest integration configs and npm script, excludes integration tests from default runs, introduces integration tests for ENS GraphQL (accounts, domains, registry, queries), and provides test helpers, devnet constants, syntax-highlighting, and a global health-check setup.

Changes

Cohort / File(s) Summary
Vitest config updates (exclude integration tests)
apps/ensadmin/vitest.config.ts, apps/ensindexer/vitest.config.ts, apps/ensrainbow/vitest.config.ts, apps/fallback-ensapi/vitest.config.ts, apps/ensapi/vitest.config.ts, packages/datasources/vitest.config.ts, packages/ens-referrals/vitest.config.ts, packages/ensnode-react/vitest.config.ts, packages/ensnode-sdk/vitest.config.ts, packages/ensrainbow-sdk/vitest.config.ts, packages/namehash-ui/vitest.config.ts, packages/ponder-metadata/vitest.config.ts, packages/ponder-sdk/vitest.config.ts
Imported configDefaults from vitest/config and extended each project's test.exclude with "**/*.integration.test.ts" to skip integration tests by default.
Integration test runner configs & scripts
vitest.integration.config.ts, apps/ensapi/vitest.integration.config.ts, apps/ensapi/package.json, package.json
Added root integration Vitest config selecting per-app/project integration configs, app-level integration project (include & globalSetup), app dev-deps, and a top-level test:integration npm script.
ENS GraphQL integration tests
apps/ensapi/src/graphql-api/schema/account.integration.test.ts, apps/ensapi/src/graphql-api/schema/domain.integration.test.ts, apps/ensapi/src/graphql-api/schema/query.integration.test.ts, apps/ensapi/src/graphql-api/schema/registry.integration.test.ts
New integration test suites validating Account.domains, Domain.subdomains, Query (root/domains/domain-by-name), and Registry domains against devnet expectations.
Test infrastructure & helpers
apps/ensapi/src/test/integration/devnet-names.ts, apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts, apps/ensapi/src/test/integration/global-setup.ts, apps/ensapi/src/test/integration/graphql-utils.ts, apps/ensapi/src/test/integration/highlight.ts
Added devnet name constants, a GraphQL client & gql re-export, a global health-check setup(), GraphQL utilities (flattenConnection, request, types), and Prism/Chalk-based highlight helpers for GraphQL/JSON logging.
Docs/docblock update
apps/ensapi/src/graphql-api/lib/get-domain-by-interpreted-name.ts
Expanded docblock describing lookup semantics and edge cases; no functional/signature changes.

Sequence Diagram(s)

sequenceDiagram
    participant Runner as "Vitest (integration project)"
    participant Setup as "global-setup.ts (health-check)"
    participant Client as "Test GraphQL client"
    participant ENSAPI as "ensapi GraphQL server"
    participant Datasource as "Datasource / ENSv2 Registry"

    Runner->>Setup: invoke setup()
    Setup->>Client: send introspection-style query
    Client->>ENSAPI: HTTP GraphQL request
    ENSAPI->>Datasource: query registry / resolve names
    Datasource-->>ENSAPI: return domain/registry data
    ENSAPI-->>Client: GraphQL response
    Client-->>Setup: return success
    Runner->>Client: run integration tests (queries)
    Client->>ENSAPI: execute test queries
    ENSAPI->>Datasource: fetch/resolve domain data
    Datasource-->>ENSAPI: data
    ENSAPI-->>Client: responses for assertions
    Client-->>Runner: test results
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

ensnode-sdk

Poem

🐰
I hopped through configs, tests, and light,
Health-checks first, then queries bright,
Devnet names catalogued with care,
GraphQL dances in the devnet air,
🥕 tests pass — a carrot for the hare.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title "ENSv2 GraphQL API Devnet Integration Test Scaffolding" clearly and accurately summarizes the main change: adding integration test infrastructure for the ENSv2 GraphQL API with devnet support.
Description check ✅ Passed The PR description comprehensively covers all required template sections: Summary (concrete changes listed), Why (motivation for integration test coverage), Testing (how to run tests and evidence), Pre-Review Checklist (all blocking items checked), and includes additional valuable context like design decisions, risk analysis, and scope management.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/graphql-api-integration-test

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 26, 2026

Greptile Summary

This PR establishes integration test scaffolding for the ENSv2 GraphQL API (ensapi), adding a separate vitest integration project, shared test utilities (GraphQL client, request helper, syntax highlighting), devnet name fixtures, and four integration test suites covering Query, Domain, Account, and Registry resolvers. All 12 existing project vitest.config.ts files are updated to exclude *.integration.test.ts from unit test runs, correctly spreading configDefaults.exclude to preserve default exclusions.

Key findings:

  • Logic bug: The "requires the name filter" test in query.integration.test.ts is missing await before expect(...).rejects.toThrow(...), causing the assertion promise to be silently discarded — the test will always pass regardless of actual behavior.
  • Misleading error message: global-setup.ts defines ENSAPI_GRAPHQL_URL reading from process.env.ENSAPI_GRAPHQL_URL, but the actual GraphQLClient in ensnode-graphql-api-client.ts uses process.env.ENSAPI_GRAPHQL_API_URL. A mismatch between these two env variable names means the health-check error message will display the wrong URL when a custom endpoint is configured.
  • Type narrowing: The .filter((l) => l !== null) in devnet-names.ts lacks a type predicate, so DEVNET_ETH_LABELS is inferred as (string | null)[] instead of string[].
  • The JSDoc for getDomainIdByInterpretedName mentions "two intentional differences" but only explicitly numbers one of them.

Confidence Score: 4/5

  • Safe to merge with minor fixes — no production runtime behavior changes; issues are confined to test infrastructure correctness.
  • The PR adds purely additive test infrastructure with no changes to runtime code (aside from a documentation-only JSDoc update). The most impactful bug is a missing await that causes one test assertion to never execute, but this only affects test coverage quality rather than production behavior. The env variable mismatch in the health-check error message is misleading but non-blocking. The vitest config changes are mechanically consistent across all 12 projects and correctly use configDefaults.exclude.
  • apps/ensapi/src/graphql-api/schema/query.integration.test.ts (missing await), apps/ensapi/src/test/integration/global-setup.ts (wrong env variable name in error message)

Important Files Changed

Filename Overview
apps/ensapi/src/graphql-api/schema/query.integration.test.ts Adds integration tests for Query.root, Query.domains, and Query.domain; contains a missing await on a .rejects.toThrow() assertion that causes the "requires the name filter" test to always pass silently.
apps/ensapi/src/test/integration/global-setup.ts Health-check setup for integration tests; references env var ENSAPI_GRAPHQL_URL in the error message while the actual GraphQL client is configured via the distinct ENSAPI_GRAPHQL_API_URL variable, causing misleading error output.
apps/ensapi/src/test/integration/devnet-names.ts Defines devnet name fixtures and DEVNET_ETH_LABELS; the .filter() call lacks a type predicate, resulting in an inferred type of `(string
apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts Minimal shared GraphQL client; clean, exports gql tag and a configured GraphQLClient instance using ENSAPI_GRAPHQL_API_URL.
apps/ensapi/src/test/integration/graphql-utils.ts Provides request() wrapper with pretty-printed logging and flattenConnection() for GraphQL cursor connections; implementation is clean and correct.
apps/ensapi/src/test/integration/highlight.ts Prism.js + chalk syntax highlighting for GraphQL and JSON output in test logs; handles nested token content recursively, looks correct.
apps/ensapi/vitest.integration.config.ts Defines the ensapi integration test project config with the correct include glob and global setup; straightforward.
vitest.integration.config.ts Root integration test workspace config; discovers per-app/package integration vitest configs and sets FORCE_COLOR for chalk output.
apps/ensapi/src/graphql-api/lib/get-domain-by-interpreted-name.ts Documentation-only change adding a detailed JSDoc comment; the comment mentions two differences but only explicitly numbers one — a minor documentation inconsistency.
apps/ensapi/src/graphql-api/schema/account.integration.test.ts Integration tests for Account.domains covering default and transferred-owner scenarios; assertions correctly use .toContain() for subset checking.
apps/ensapi/src/graphql-api/schema/domain.integration.test.ts Integration test for Domain.subdomains on the .eth TLD; verifies all known devnet ETH labels appear in subdomain results.
apps/ensapi/src/graphql-api/schema/registry.integration.test.ts Integration test for Registry.domains verifying the ETH registry contains all known devnet labels; straightforward and correct.
apps/ensapi/vitest.config.ts Updated to exclude *.integration.test.ts files using configDefaults.exclude spread, consistent with all other project configs.
apps/ensapi/package.json Adds test:integration script and new devDependencies (chalk, prismjs, graphql-request, @types/prismjs); changes are appropriate for the integration test infrastructure.

Sequence Diagram

sequenceDiagram
    participant Dev as Developer
    participant Vitest as Vitest (integration)
    participant GS as global-setup.ts
    participant Client as GraphQLClient
    participant ENSApi as ensapi (live)

    Dev->>Vitest: pnpm test:integration
    Vitest->>GS: setup()
    GS->>Client: request(__schema introspection)
    Client->>ENSApi: POST /api/graphql
    alt ENSApi reachable
        ENSApi-->>Client: schema response
        Client-->>GS: ok
        GS-->>Vitest: setup complete
        Vitest->>Vitest: run *.integration.test.ts suites
        Note over Vitest: query / domain / account / registry tests
        Vitest->>Client: request(gql`...`, variables)
        Client->>ENSApi: POST /api/graphql
        ENSApi-->>Client: GraphQL response
        Client-->>Vitest: typed result
        Vitest->>Vitest: assert expectations
    else ENSApi unreachable
        ENSApi-->>Client: connection error
        Client-->>GS: throws
        GS-->>Vitest: throw Error("health check failed …")
        Vitest->>Dev: fast-fail with actionable message
    end
Loading

Last reviewed commit: e32005f

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

28 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR establishes integration test scaffolding for the ENSv2 GraphQL API to enable automated verification against real indexed devnet data. The PR adds a separate integration test project using vitest, creates reusable test utilities with syntax highlighting, defines devnet name fixtures, and implements initial integration tests covering Query, Domain, Account, and Registry resolvers.

Changes:

  • Added integration test infrastructure with vitest configs, global setup health check, GraphQL client, and syntax-highlighted request/response logging
  • Updated all 12 project vitest configs to exclude *.integration.test.ts from unit test runs
  • Added initial integration tests for GraphQL schema covering root queries, domain lookups, account domains, and registry domains
  • Enhanced documentation for getDomainIdByInterpretedName explaining forward traversal semantics and alias handling

Reviewed changes

Copilot reviewed 27 out of 28 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
vitest.integration.config.ts Root-level integration test config defining project glob patterns and FORCE_COLOR env
package.json Added test:integration script to run integration tests separately
vitest.config.ts (12 projects) Added integration test exclusion pattern to all unit test configs
apps/ensapi/vitest.integration.config.ts Integration test project config for ensapi with global setup
apps/ensapi/package.json Added test:integration script and dev dependencies (chalk, prismjs, graphql-request)
apps/ensapi/src/test/integration/global-setup.ts Health check via introspection query before tests run
apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts Shared GraphQLClient instance and gql tag export
apps/ensapi/src/test/integration/graphql-utils.ts Request helper with logging and connection flattening utility
apps/ensapi/src/test/integration/highlight.ts Prism+chalk syntax highlighting for GraphQL and JSON
apps/ensapi/src/test/integration/devnet-names.ts Devnet name fixtures with canonical mappings
apps/ensapi/src/graphql-api/schema/*.integration.test.ts Integration tests for Query, Domain, Account, and Registry resolvers
apps/ensapi/src/graphql-api/lib/get-domain-by-interpreted-name.ts Enhanced JSDoc explaining forward traversal, expired names, and alias behavior
pnpm-lock.yaml Lock file updates for new dev dependencies
Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 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/schema/account.integration.test.ts`:
- Around line 45-58: The test's hardcoded expected domains array should be
generated from the central fixture DEVNET_NAMES instead of duplicating values;
update the assertion in account.integration.test.ts to derive expected domain
names by mapping/filtering DEVNET_NAMES (and, if ownership info exists, filter
by the account owner) to produce the same list used in the test, or at minimum
replace the literal array with a short comment referencing devnet-names.ts and a
TODO to derive from DEVNET_NAMES; key symbols to change are the expected
variable in account.integration.test.ts and the DEVNET_NAMES export in
devnet-names.ts.

In `@apps/ensapi/src/graphql-api/schema/query.integration.test.ts`:
- Around line 75-78: The test "requires the name filter" currently calls
expect(request(gql`{ domains { edges { node { id }} } }`)).rejects.toThrow(...)
without awaiting the assertion; update the spec so the rejection assertion is
awaited (i.e., prepend await to the expect(...).rejects.toThrow call) so the
test actually waits for the promise rejection from request/gql and fails
correctly if no error is thrown.
- Around line 81-99: Rename the test "sees both .eth Domains" to reflect current
devnet behavior (e.g., "sees v2 .eth Domain only"), update assertions to
explicitly assert the v2 domain exists (expect(v2EthDomain).toBeDefined() or
toMatchObject(...)) and v1 is absent (keep expect(v1EthDomain).toBeUndefined()),
and remove the dead commented expectations referring to V1_ETH_DOMAIN_ID and
label; locate and change the test declaration it("sees both .eth Domains", ...)
and the variables v1EthDomain / v2EthDomain in the same block.

In `@apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts`:
- Around line 5-8: Export the ENSAPI_GRAPHQL_API_URL constant so other modules
(e.g., global-setup.ts) can import the canonical env var name and default;
specifically, change the declaration of ENSAPI_GRAPHQL_API_URL to be exported
(export const ENSAPI_GRAPHQL_API_URL = ...) and then keep using that same
exported symbol when creating the GraphQLClient (export const client = new
GraphQLClient(ENSAPI_GRAPHQL_API_URL)); this avoids duplicating the env variable
name and default value across files.

In `@apps/ensapi/src/test/integration/global-setup.ts`:
- Line 3: The test setup uses ENSAPI_GRAPHQL_URL while the client module uses
ENSAPI_GRAPHQL_API_URL, causing mismatched runtime URLs; fix by exporting the
canonical URL constant from ensnode-graphql-api-client.ts (export const
ENSAPI_GRAPHQL_API_URL = process.env.ENSAPI_GRAPHQL_API_URL ??
"http://localhost:4334/api/graphql") and then import and use that exported
ENSAPI_GRAPHQL_API_URL in global-setup.ts instead of declaring
ENSAPI_GRAPHQL_URL locally, and also update any related error messages to
reference ENSAPI_GRAPHQL_API_URL so both modules use the exact same identifier
and value.

In `@apps/ensapi/src/test/integration/graphql-utils.ts`:
- Around line 15-21: The request function calls document.toString() unguarded;
update request(document) to handle RequestDocument variants by detecting if
document is a DocumentNode (e.g., check for an object with a "kind" property and
"definitions") and, in that case, call print(document) directly, otherwise treat
document as a string and call parse(document.toString()) before print; adjust
the creation of query to use either print(document as DocumentNode) or
print(parse(documentString)) so DocumentNode inputs produce valid GraphQL text
and string inputs continue to be parsed.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8025915 and e32005f.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (27)
  • apps/ensadmin/vitest.config.ts
  • apps/ensapi/package.json
  • apps/ensapi/src/graphql-api/lib/get-domain-by-interpreted-name.ts
  • apps/ensapi/src/graphql-api/schema/account.integration.test.ts
  • apps/ensapi/src/graphql-api/schema/domain.integration.test.ts
  • apps/ensapi/src/graphql-api/schema/query.integration.test.ts
  • apps/ensapi/src/graphql-api/schema/registry.integration.test.ts
  • apps/ensapi/src/test/integration/devnet-names.ts
  • apps/ensapi/src/test/integration/ensnode-graphql-api-client.ts
  • apps/ensapi/src/test/integration/global-setup.ts
  • apps/ensapi/src/test/integration/graphql-utils.ts
  • apps/ensapi/src/test/integration/highlight.ts
  • apps/ensapi/vitest.config.ts
  • apps/ensapi/vitest.integration.config.ts
  • apps/ensindexer/vitest.config.ts
  • apps/ensrainbow/vitest.config.ts
  • apps/fallback-ensapi/vitest.config.ts
  • package.json
  • packages/datasources/vitest.config.ts
  • packages/ens-referrals/vitest.config.ts
  • packages/ensnode-react/vitest.config.ts
  • packages/ensnode-sdk/vitest.config.ts
  • packages/ensrainbow-sdk/vitest.config.ts
  • packages/namehash-ui/vitest.config.ts
  • packages/ponder-metadata/vitest.config.ts
  • packages/ponder-sdk/vitest.config.ts
  • vitest.integration.config.ts

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Copilot AI review requested due to automatic review settings February 26, 2026 21:07
@vercel vercel bot temporarily deployed to Preview – ensnode.io February 26, 2026 21:07 Inactive
@vercel vercel bot temporarily deployed to Preview – admin.ensnode.io February 26, 2026 21:07 Inactive
@vercel vercel bot temporarily deployed to Preview – ensrainbow.io February 26, 2026 21:07 Inactive
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@vercel vercel bot temporarily deployed to Preview – ensrainbow.io February 26, 2026 21:07 Inactive
@vercel vercel bot temporarily deployed to Preview – ensnode.io February 26, 2026 21:07 Inactive
@vercel vercel bot temporarily deployed to Preview – admin.ensnode.io February 26, 2026 21:07 Inactive
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • pnpm-lock.yaml: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (1)
apps/ensapi/src/graphql-api/schema/query.integration.test.ts (1)

81-105: 🧹 Nitpick | 🔵 Trivial

Test name does not match assertions.

The test is titled "sees both .eth Domains" but actually asserts that v1EthDomain is undefined (line 93) and only validates the v2 domain. Consider renaming to reflect current devnet behavior (e.g., "returns the ENSv2 .eth domain") and removing the commented-out expectations to reduce confusion.

Suggested cleanup
-  it("sees both .eth Domains", async () => {
+  it("returns the ENSv2 .eth domain on current devnet", async () => {
     const result = await request<QueryDomainsResult>(QueryDomains, { name: "eth" });

     const domains = flattenConnection(result.domains);

     // there's at least a v2 'eth' domain
     expect(domains.length).toBeGreaterThanOrEqual(1);

     const v1EthDomain = domains.find((d) => d.__typename === "ENSv1Domain" && d.name === "eth");
     const v2EthDomain = domains.find((d) => d.__typename === "ENSv2Domain" && d.name === "eth");

-    // TODO: the v1 eth label should surely exist but i don't see it in devnet at the moment?
+    // v1 eth domain is not present in current devnet
     expect(v1EthDomain).toBeUndefined();
-    // expect(v1EthDomain).toMatchObject({
-    //   id: V1_ETH_DOMAIN_ID,
-    //   name: "eth",
-    //   label: { interpreted: "eth" },
-    // });

     expect(v2EthDomain).toMatchObject({
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/ensapi/src/graphql-api/schema/query.integration.test.ts` around lines 81
- 105, The test title "sees both .eth Domains" is misleading because the
assertions expect v1EthDomain to be undefined and only validate v2EthDomain;
update the test name string in the it(...) call to reflect current behavior
(e.g., "returns the ENSv2 .eth domain") and remove the commented-out v1
expectations to avoid confusion; ensure the assertions still reference
v1EthDomain and v2EthDomain and that v2 validation uses V2_ETH_DOMAIN_ID, name
"eth", and label.interpreted "eth" as before.
🤖 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/test/integration/global-setup.ts`:
- Around line 1-23: Run the repository formatter on this file to fix the CI
formatting error; specifically format
apps/ensapi/src/test/integration/global-setup.ts (trim trailing whitespace and
apply project-prettierrc/ESLint rules) so the existing setup function,
ENSAPI_GRAPHQL_URL constant, and the client.request GraphQL template literal
match the repo style; after formatting, commit the updated file and push to
resolve the pipeline failure.

---

Duplicate comments:
In `@apps/ensapi/src/graphql-api/schema/query.integration.test.ts`:
- Around line 81-105: The test title "sees both .eth Domains" is misleading
because the assertions expect v1EthDomain to be undefined and only validate
v2EthDomain; update the test name string in the it(...) call to reflect current
behavior (e.g., "returns the ENSv2 .eth domain") and remove the commented-out v1
expectations to avoid confusion; ensure the assertions still reference
v1EthDomain and v2EthDomain and that v2 validation uses V2_ETH_DOMAIN_ID, name
"eth", and label.interpreted "eth" as before.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e32005f and e992be5.

📒 Files selected for processing (2)
  • apps/ensapi/src/graphql-api/schema/query.integration.test.ts
  • apps/ensapi/src/test/integration/global-setup.ts

@vercel vercel bot temporarily deployed to Preview – admin.ensnode.io February 26, 2026 23:08 Inactive
@vercel vercel bot temporarily deployed to Preview – ensrainbow.io February 26, 2026 23:08 Inactive
@vercel vercel bot temporarily deployed to Preview – ensnode.io February 26, 2026 23:08 Inactive
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
apps/ensapi/src/graphql-api/schema/query.integration.test.ts (1)

92-98: 🧹 Nitpick | 🔵 Trivial

Consider removing dead code or converting to a tracking issue.

The commented-out assertions and TODO for the v1 eth domain add noise. If this is a known devnet limitation, consider either:

  1. Removing the dead code entirely and documenting the limitation elsewhere
  2. Opening an issue to track when v1 eth domain should appear
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/ensapi/src/graphql-api/schema/query.integration.test.ts` around lines 92
- 98, Remove the noisy commented-out assertions and TODO around the v1 eth
domain in the test (references: v1EthDomain and V1_ETH_DOMAIN_ID); either delete
the commented expect block entirely or replace it with a brief comment pointing
to a newly created tracking issue that documents the devnet limitation and
desired behavior, and ensure the test only asserts the current known state
(v1EthDomain is undefined) so the test stays clean and actionable.
🤖 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/test/integration/global-setup.ts`:
- Around line 3-4: The constant name ENSAPI_GRAPHQL_URL is inconsistent with the
environment variable ENSAPI_GRAPHQL_API_URL; rename the constant to
ENSAPI_GRAPHQL_API_URL (or alternatively read from ENSAPI_GRAPHQL_URL) so names
match, update the assignment that uses process.env.ENSAPI_GRAPHQL_API_URL
accordingly, and modify any error/log messages that refer to ENSAPI_GRAPHQL_URL
to use the new ENSAPI_GRAPHQL_API_URL identifier to keep names consistent across
the file.

In `@apps/ensapi/src/test/integration/graphql-utils.ts`:
- Around line 15-22: Replace the hardcoded "Document" check in the
isDocumentNode type guard with the graphql Kind enum: import Kind from the
'graphql' package and change the condition to compare obj.kind === Kind.DOCUMENT
so the function isDocumentNode(obj: any): obj is DocumentNode uses Kind.DOCUMENT
for robustness and maintainability.

---

Duplicate comments:
In `@apps/ensapi/src/graphql-api/schema/query.integration.test.ts`:
- Around line 92-98: Remove the noisy commented-out assertions and TODO around
the v1 eth domain in the test (references: v1EthDomain and V1_ETH_DOMAIN_ID);
either delete the commented expect block entirely or replace it with a brief
comment pointing to a newly created tracking issue that documents the devnet
limitation and desired behavior, and ensure the test only asserts the current
known state (v1EthDomain is undefined) so the test stays clean and actionable.

ℹ️ Review info

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e992be5 and 3e91b4a.

📒 Files selected for processing (5)
  • apps/ensapi/src/graphql-api/lib/get-domain-by-interpreted-name.ts
  • apps/ensapi/src/graphql-api/schema/query.integration.test.ts
  • apps/ensapi/src/test/integration/devnet-names.ts
  • apps/ensapi/src/test/integration/global-setup.ts
  • apps/ensapi/src/test/integration/graphql-utils.ts

Copilot AI review requested due to automatic review settings February 26, 2026 23:21
@vercel vercel bot temporarily deployed to Preview – ensnode.io February 26, 2026 23:21 Inactive
@vercel vercel bot temporarily deployed to Preview – admin.ensnode.io February 26, 2026 23:21 Inactive
@vercel vercel bot temporarily deployed to Preview – ensrainbow.io February 26, 2026 23:21 Inactive
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated no new 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.

Copy link
Member

@lightwalker-eth lightwalker-eth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shrugs Great to see this! Shared one key point of feedback. Please take the lead to merge when ready 👍

* This is because this traversal is designed to support access to a Domain via API—not Protocol
* Acceleration—and consumers likely want to reference Domains regardless of Registration status.
*
* This means that not every Domain returned by this function is accessible by ENS Forward Resolution:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ENS forward resolution makes no consideration of expiry status -- the resolution protocol has no conception of expiry (and more broadly, has no conception of ownership either)

For example, consider nrg.eth which has no active registration as I write this: https://app.ens.domains/nrg.eth/register

But you can still resolve records for this name based on what was configured for it by its previous owner:

Did you see something that's changing this for ENSv2?

const v1EthDomain = domains.find((d) => d.__typename === "ENSv1Domain" && d.name === "eth");
const v2EthDomain = domains.find((d) => d.__typename === "ENSv2Domain" && d.name === "eth");

// TODO: the v1 eth label should surely exist but i don't see it in devnet at the moment?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's strange

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants