enskit omnigraph: react bindings, id refactoring, schema improvements#1867
Open
enskit omnigraph: react bindings, id refactoring, schema improvements#1867
Conversation
🦋 Changeset detectedLatest commit: 20bdae6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 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 |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates ID/CAIP helpers into Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant App as React App
participant Provider as OmnigraphProvider
participant URQL as URQL Client
participant Cache as Graphcache Exchange
participant OmnigraphAPI as Omnigraph API
participant DB as Database
Browser->>App: navigate route (e.g. /domain/:name)
App->>Provider: render with EnsNodeClient
Provider->>URQL: memoize/create client(ensNodeUrl)
App->>URQL: useOmnigraphQuery(query, vars)
URQL->>Cache: resolve query (keys, connection resolvers)
alt cache miss
Cache->>OmnigraphAPI: POST /api/omnigraph (query + vars)
OmnigraphAPI->>DB: load data (by id or by.contract)
DB-->>OmnigraphAPI: return rows
OmnigraphAPI-->>Cache: return data
Cache-->>URQL: deliver result (cacheOutcome meta)
else cache hit
Cache-->>URQL: return cached result (cacheOutcome meta)
end
URQL-->>App: data | loading | error
App-->>Browser: render UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Continues the Omnigraph client work by splitting out an enssdk Omnigraph module (including a distributable schema artifact), adding a new enskit React-facing wrapper around Omnigraph using urql/graphcache, and introducing a Vite-based example app to demonstrate typed Omnigraph queries.
Changes:
- Add
enssdkOmnigraph schema/module structure, including generated SDL artifacts and an integration test harness. - Introduce new
enskitpackage providing React hooks/provider + urql client setup for Omnigraph. - Expand the monorepo workspace to include
examples/*and add anenskit-react-exampleVite app.
Reviewed changes
Copilot reviewed 32 out of 37 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.integration.config.ts | Updates integration workspace project discovery and sets ENSNODE_URL env default. |
| pnpm-workspace.yaml | Adds examples/* to the pnpm workspace. |
| pnpm-lock.yaml | Adds/updates dependencies for new packages/example (urql/graphcache, vite, react-router, etc.). |
| packages/enssdk/vitest.integration.config.ts | Adds package-level integration test config for enssdk. |
| packages/enssdk/vitest.config.ts | Simplifies vitest config and excludes integration tests. |
| packages/enssdk/src/omnigraph/schema.ts | Adds runtime-built GraphQL schema from generated SDL. |
| packages/enssdk/src/omnigraph/module.ts | Moves Omnigraph query module implementation into a dedicated module file. |
| packages/enssdk/src/omnigraph/module.test.ts | Updates unit test import to new module location. |
| packages/enssdk/src/omnigraph/module.integration.test.ts | Adds integration test hitting a real ENSNode Omnigraph endpoint. |
| packages/enssdk/src/omnigraph/index.ts | Re-exports Omnigraph graphql helpers, module, and schema. |
| packages/enssdk/src/omnigraph/generated/schema.graphql | Updates generated schema nullability for relay edges/nodes. |
| packages/enssdk/src/omnigraph/generated/schema-sdl.ts | Adds generated SDL string for runtime schema construction. |
| packages/enssdk/src/omnigraph/generated/graphql-env.d.ts | Updates gql.tada environment types to match new nullability. |
| packages/enssdk/package.json | Exports and publishes schema.graphql alongside existing Omnigraph exports; adds @types/node devDep. |
| packages/enskit/vitest.integration.config.ts | Adds enskit integration test config (jsdom). |
| packages/enskit/vitest.config.ts | Adds enskit unit test config (jsdom) excluding integration tests. |
| packages/enskit/tsup.config.ts | Adds tsup build config for bundling enskit’s React Omnigraph entrypoint. |
| packages/enskit/tsconfig.json | Adds TypeScript config for enskit package build. |
| packages/enskit/src/react/omnigraph/provider.tsx | Adds React provider wiring an urql Client into context. |
| packages/enskit/src/react/omnigraph/index.ts | Public React Omnigraph entrypoint re-exporting gql.tada helpers + hooks/provider. |
| packages/enskit/src/react/omnigraph/hooks.ts | Adds useOmnigraphQuery hook wrapper over urql useQuery. |
| packages/enskit/src/react/omnigraph/client.ts | Adds urql Client factory using graphcache + schema introspection. |
| packages/enskit/package.json | Converts enskit into a publishable ESM package with exports, build, lint, and test scripts. |
| packages/enskit/biome.jsonc | Adds Biome config for enskit package. |
| examples/enskit-react-example/vite.config.ts | Adds Vite config for the new React example app. |
| examples/enskit-react-example/tsconfig.json | Adds TS config enabling gql.tada plugin against the Omnigraph schema file. |
| examples/enskit-react-example/src/vite-env.d.ts | Adds Vite client types reference. |
| examples/enskit-react-example/src/main.tsx | Adds example app bootstrap using OmnigraphProvider + routing. |
| examples/enskit-react-example/src/DomainView.tsx | Adds example view using typed gql.tada queries/fragments + useOmnigraphQuery. |
| examples/enskit-react-example/package.json | Adds example app package metadata and dependencies. |
| examples/enskit-react-example/index.html | Adds example app HTML entrypoint. |
| apps/ensapi/vitest.integration.config.ts | Removes integration globalSetup from ensapi’s integration config. |
| apps/ensapi/src/test/integration/omnigraph-api-client.ts | Makes ENSNODE_URL required (non-null asserted) for integration client URL. |
| apps/ensapi/src/test/integration/global-setup.ts | Removes integration health-check global setup implementation. |
| apps/ensapi/src/omnigraph-api/lib/write-graphql-schema.ts | Updates schema writer to emit both schema.graphql and schema-sdl.ts. |
| apps/ensapi/src/omnigraph-api/builder.ts | Sets global relay edge/node nullability defaults and options. |
| apps/ensapi/package.json | Removes test:integration script. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/enskit-react-example/src/DomainView.tsx`:
- Around line 43-47: The route param is being unsafely cast to the branded type
InterpretedName when calling useOmnigraphQuery; instead validate the value from
useParams() with a Zod schema before using it. Create or reuse a Zod schema for
ENS names (e.g., nameSchema) and call nameSchema.parse(name) (or safeParse and
handle errors) to produce a validated value, then pass that validated value into
the variables for DomainByNameQuery in the useOmnigraphQuery call; remove the
"as InterpretedName" assertion and ensure error handling or fallback behavior if
validation fails.
In `@packages/enskit/src/react/omnigraph/hooks.ts`:
- Around line 8-13: UseOmnigraphQueryArgs currently makes variables optional for
all documents; change its variables property to a conditional type like the
QueryOptions pattern so that when the document's Variables type has keys
variables is required and when it doesn't it's optional/undefined. Update the
type alias UseOmnigraphQueryArgs<Data, Variables extends AnyVariables =
AnyVariables> (and any related usages/casts that mask the mismatch) to follow
the V extends Record<string, unknown> ? keyof V extends never ? { variables?: V
} : { variables: V } : { variables?: undefined } pattern (or equivalent)
referencing the DocumentNode|TadaDocumentNode generic so required GraphQL
variables are enforced at compile time. Ensure any current casts around
UseOmnigraphQueryArgs are removed or adjusted to preserve the stricter typing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7420ab6d-8c49-49c3-9486-7289b580ec83
⛔ Files ignored due to path filters (4)
packages/enssdk/src/omnigraph/generated/graphql-env.d.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema-sdl.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
apps/ensapi/package.jsonapps/ensapi/src/omnigraph-api/builder.tsapps/ensapi/src/omnigraph-api/lib/write-graphql-schema.tsapps/ensapi/src/test/integration/global-setup.tsapps/ensapi/src/test/integration/omnigraph-api-client.tsapps/ensapi/vitest.integration.config.tsexamples/enskit-react-example/index.htmlexamples/enskit-react-example/package.jsonexamples/enskit-react-example/src/DomainView.tsxexamples/enskit-react-example/src/main.tsxexamples/enskit-react-example/src/vite-env.d.tsexamples/enskit-react-example/tsconfig.jsonexamples/enskit-react-example/vite.config.tspackages/enskit/biome.jsoncpackages/enskit/package.jsonpackages/enskit/src/react/omnigraph/client.tspackages/enskit/src/react/omnigraph/hooks.tspackages/enskit/src/react/omnigraph/index.tspackages/enskit/src/react/omnigraph/provider.tsxpackages/enskit/tsconfig.jsonpackages/enskit/tsup.config.tspackages/enskit/vitest.config.tspackages/enskit/vitest.integration.config.tspackages/enssdk/package.jsonpackages/enssdk/src/omnigraph/index.tspackages/enssdk/src/omnigraph/module.integration.test.tspackages/enssdk/src/omnigraph/module.test.tspackages/enssdk/src/omnigraph/module.tspackages/enssdk/src/omnigraph/schema.tspackages/enssdk/vitest.config.tspackages/enssdk/vitest.integration.config.tspnpm-workspace.yamlvitest.integration.config.ts
💤 Files with no reviewable changes (3)
- apps/ensapi/package.json
- apps/ensapi/vitest.integration.config.ts
- apps/ensapi/src/test/integration/global-setup.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
- generate minified introspection via @urql/introspection instead of raw SDL - replace runtime introspectionFromSchema + buildSchema with pre-computed introspection - add graphcache local resolvers for domain, account, registry, resolver - disable yoga's built-in CORS (handled by Hono middleware) - disable noUnusedFunctionParameters for graphcache resolver file Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…Id encoding as hex when should be string number
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 96 out of 101 changed files in this pull request and generated 11 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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/enskit-react-example/src/main.tsx`:
- Around line 26-28: Replace the plain <a href="/domain/eth"> and <a
href="/registry"> anchors inside the <nav> with client-side Link components so
navigation stays in-app and preserves OmnigraphProvider/URQL state; import Link
from your router library (e.g., react-router-dom's Link) and use <Link
to="/domain/eth">Domains</Link> and <Link to="/registry">Registry Cache
Demo</Link> in place of the href anchors in the navigation.
In `@examples/enskit-react-example/src/RegistryView.tsx`:
- Line 86: In RegistryView (the JSX rendering the registry IDs), remove the
literal dollar sign and template delimiters and render the values as JSX
expressions instead of a template string; specifically replace the templated
markup around byIdResult.data?.registry?.id and byOwnerResult.data?.registry?.id
so the dollar sign is not output and the values are rendered via JSX expression
evaluation.
- Around line 25-28: The CacheStatus component currently treats cacheOutcome as
truthy/falsy causing "miss" and "partial" to be shown as a hit; update
CacheStatus to read result.operation.context.meta.cacheOutcome and explicitly
branch on its string values: if cacheOutcome === "hit" render a green "[hit]",
if cacheOutcome === "miss" render an orange/red "[miss]", if cacheOutcome ===
"partial" render a distinct color (e.g., orange) and label "[partial]", and
fallback to a neutral "[unknown]" for any other value.
In `@packages/enskit/src/react/omnigraph/client.ts`:
- Around line 21-25: The JSDoc block at the top of
packages/enskit/src/react/omnigraph/client.ts is incomplete and has a redundant
`@returns`; either finish the summary so it reads as a complete sentence and adds
meaningful detail, or remove the JSDoc block entirely (and drop the redundant
`@returns` line) so the function signature stands alone; locate the comment
immediately preceding the client function (the JSDoc starting "Many of our
resolvers allow for exact") and update it accordingly.
In `@packages/enssdk/src/lib/helpers.ts`:
- Around line 3-8: The JSDoc above uint256ToHex32 is incorrect (references
AssetId/CAIP-19); update the comment to accurately describe that uint256ToHex32
takes a bigint and returns a 32-byte (256-bit) hex string (using toHex with {
size: 32 }) — keep the signature and implementation (export const uint256ToHex32
= (num: bigint): Hex => toHex(num, { size: 32 })) unchanged and ensure the doc
mentions input type bigint and returned Hex format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bad450e-70df-4126-ab62-8e49f5c81d2d
⛔ Files ignored due to path filters (4)
packages/enssdk/src/omnigraph/generated/graphql-env.d.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/introspection.tsis excluded by!**/generated/**packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (74)
.changeset/warm-ghosts-enjoy.mdapps/ensapi/package.jsonapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database-v1.tsapps/ensapi/src/lib/ensanalytics/referrer-leaderboard/database.tsapps/ensapi/src/omnigraph-api/lib/write-graphql-schema.tsapps/ensapi/src/omnigraph-api/schema/account-id.tsapps/ensapi/src/omnigraph-api/schema/account.integration.test.tsapps/ensapi/src/omnigraph-api/schema/account.tsapps/ensapi/src/omnigraph-api/schema/permissions.integration.test.tsapps/ensapi/src/omnigraph-api/schema/permissions.tsapps/ensapi/src/omnigraph-api/schema/query.integration.test.tsapps/ensapi/src/omnigraph-api/schema/query.tsapps/ensapi/src/omnigraph-api/schema/registry-permissions-user.tsapps/ensapi/src/omnigraph-api/schema/registry.tsapps/ensapi/src/omnigraph-api/schema/resolver-permissions-user.tsapps/ensapi/src/omnigraph-api/schema/resolver.tsapps/ensapi/src/omnigraph-api/yoga.tsapps/ensapi/src/test/integration/find-domains/domain-pagination-queries.tsapps/ensapi/src/test/integration/find-events/event-pagination-queries.tsapps/ensindexer/package.jsonapps/ensindexer/src/lib/ensv2/event-db-helpers.tsapps/ensindexer/src/lib/ensv2/registration-db-helpers.tsapps/ensindexer/src/lib/protocol-acceleration/resolver-db-helpers.tsapps/ensindexer/src/lib/tokenscope/nft-issuers.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/BaseRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/ENSv1Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/NameWrapper.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv1/RegistrarController.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ENSv2Registry.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/ETHRegistrar.tsapps/ensindexer/src/plugins/ensv2/handlers/ensv2/EnhancedAccessControl.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv1Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ENSv2Registry.tsapps/ensindexer/src/plugins/protocol-acceleration/handlers/ThreeDNSToken.tsapps/ensindexer/src/plugins/registrars/basenames/handlers/Basenames_Registrar.tsapps/ensindexer/src/plugins/registrars/ethnames/handlers/Ethnames_Registrar.tsapps/ensindexer/src/plugins/registrars/lineanames/handlers/Lineanames_Registrar.tsapps/ensindexer/src/plugins/registrars/shared/lib/registrar-action.tsapps/ensindexer/src/plugins/registrars/shared/lib/registrar-events.tsapps/ensindexer/src/plugins/registrars/shared/lib/registration-lifecycle.tsapps/ensindexer/src/plugins/registrars/shared/lib/subregistry.tsapps/ensindexer/src/plugins/subgraph/plugins/basenames/handlers/Registrar.tsapps/ensindexer/src/plugins/subgraph/plugins/lineanames/handlers/Registrar.tsapps/ensindexer/src/plugins/subgraph/plugins/subgraph/handlers/Registrar.tsapps/ensindexer/src/plugins/subgraph/shared-handlers/NameWrapper.tsapps/ensindexer/src/plugins/tokenscope/handlers/Seaport.tsapps/ensindexer/src/plugins/tokenscope/lib/handle-nft-transfer.tsexamples/enskit-react-example/src/RegistryView.tsxexamples/enskit-react-example/src/main.tsxpackages/enskit/biome.jsoncpackages/enskit/package.jsonpackages/enskit/src/react/omnigraph/client.tspackages/ensnode-sdk/src/ens/subname-helpers.test.tspackages/ensnode-sdk/src/ens/subname-helpers.tspackages/ensnode-sdk/src/ensv2/index.tspackages/ensnode-sdk/src/index.tspackages/ensnode-sdk/src/omnigraph-api/example-queries.tspackages/ensnode-sdk/src/shared/account-id.test.tspackages/ensnode-sdk/src/shared/interpretation/index.tspackages/ensnode-sdk/src/shared/interpretation/interpret-tokenid.tspackages/ensnode-sdk/src/shared/serialize.tspackages/ensnode-sdk/src/tokenscope/assets.tspackages/enssdk/package.jsonpackages/enssdk/src/index.tspackages/enssdk/src/lib/caip.tspackages/enssdk/src/lib/helpers.test.tspackages/enssdk/src/lib/helpers.tspackages/enssdk/src/lib/ids.tspackages/enssdk/src/lib/index.tspackages/enssdk/src/lib/interpretation/index.tspackages/enssdk/src/lib/interpretation/token-id.tspackages/enssdk/src/omnigraph/index.tspackages/enssdk/src/omnigraph/introspection.tspnpm-workspace.yaml
💤 Files with no reviewable changes (4)
- packages/ensnode-sdk/src/shared/interpretation/index.ts
- packages/ensnode-sdk/src/index.ts
- packages/ensnode-sdk/src/ensv2/index.ts
- packages/ensnode-sdk/src/shared/interpretation/interpret-tokenid.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
packages/enskit/src/react/omnigraph/lib/cache-exchange.ts (1)
22-27: 🧹 Nitpick | 🔵 TrivialDrop the incomplete JSDoc and redundant
@returns.At Line 23-Line 26, the block is incomplete and the
@returnsline adds no value beyond the function signature.Proposed fix
-/** - * Many of our resolvers allow for exact - * - * `@returns` the cached data or undefined - */ const passthrough = (args: Variables, cache: Cache, info: ResolveInfo) => cache.resolve(info.parentTypeName, info.fieldName, args);As per coding guidelines, "Do not add JSDoc
@returnstags that merely restate the method summary; remove redundant documentation during PR review".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enskit/src/react/omnigraph/lib/cache-exchange.ts` around lines 22 - 27, Remove the incomplete JSDoc block above the passthrough function; delete the multi-line comment that includes the unfinished description and the redundant `@returns` tag so the function declaration `const passthrough = (args: Variables, cache: Cache, info: ResolveInfo) =>` stands without the misleading JSDoc (optionally replace it with a single-line comment like "passthrough resolver" if a brief note is desired).examples/enskit-react-example/src/main.tsx (1)
27-30:⚠️ Potential issue | 🟠 MajorUse client-side router links for internal navigation.
At Line 28 and Line 29, plain
<a href>causes full-page reloads, which resetsOmnigraphProvider/URQL in-memory cache and weakens the cache demo behavior.Proposed fix
-import { BrowserRouter, Route, Routes } from "react-router"; +import { BrowserRouter, Link, Route, Routes } from "react-router"; @@ - <a href="/domain/eth">Domains</a> | <a href="/registry">Registry Cache Demo</a> |{" "} - <a href="/pagination">Pagination Demo</a> + <Link to="/domain/eth">Domains</Link> |{" "} + <Link to="/registry">Registry Cache Demo</Link> |{" "} + <Link to="/pagination">Pagination Demo</Link>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/enskit-react-example/src/main.tsx` around lines 27 - 30, Replace the plain anchor tags inside the <nav> with client-side router links so internal navigation to /domain/eth, /registry and /pagination uses the app router (e.g., react-router-dom's <Link to="...">) instead of full-page reloads; import the router Link component at the top of main.tsx, swap the internal <a href="..."> elements for <Link to="..."> inside the nav, and leave any external links as <a> if present, so OmnigraphProvider/URQL in-memory cache is preserved during navigation.examples/enskit-react-example/src/RegistryView.tsx (2)
25-29:⚠️ Potential issue | 🟡 MinorBranch on explicit
cacheOutcomevalues.At Line 27, truthy/falsy handling can misreport status (
"miss"and"partial"currently render like success). Handle explicit states so the demo reflects cache behavior accurately.Proposed fix
function CacheStatus({ result }: { result: UseOmnigraphQueryResult[0] }) { const cacheOutcome = result.operation?.context.meta?.cacheOutcome; - if (!cacheOutcome) return <span style={{ color: "orange" }}>[uncached]</span>; - return <span style={{ color: "green" }}>[{cacheOutcome ?? "unknown"}]</span>; + if (cacheOutcome === "hit") return <span style={{ color: "green" }}>[hit]</span>; + if (cacheOutcome === "partial") return <span style={{ color: "orange" }}>[partial]</span>; + if (cacheOutcome === "miss") return <span style={{ color: "orange" }}>[miss]</span>; + return <span style={{ color: "gray" }}>[unknown]</span>; }For urql v5 + `@urql/exchange-graphcache` v9, what are the documented possible values of `result.operation.context.meta.cacheOutcome`?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/enskit-react-example/src/RegistryView.tsx` around lines 25 - 29, The CacheStatus component currently treats cacheOutcome as truthy/falsy which mislabels "miss" and "partial" as success; update CacheStatus to read result.operation.context.meta.cacheOutcome explicitly and switch on its possible values (e.g., "hit", "miss", "partial", and a fallback like undefined/"unknown") so each maps to the correct label/color (e.g., green for "hit", orange for "partial", red or orange for "miss", and a neutral label for undefined). Locate the CacheStatus function and replace the current truthy check with an explicit switch/if-else over cacheOutcome to render accurate status text and color for each known outcome.
86-86:⚠️ Potential issue | 🟡 MinorRemove the literal
$from JSX ID rendering.At Line 86 and Line 102, the dollar sign is rendered literally because this is JSX text, not a template string.
Proposed fix
- <pre>id: ${byIdResult.data?.registry?.id ?? "-"}</pre> + <pre>id: {byIdResult.data?.registry?.id ?? "-"}</pre> @@ - <pre>id: ${byContractResult.data?.registry?.id ?? "-"}</pre> + <pre>id: {byContractResult.data?.registry?.id ?? "-"}</pre>Also applies to: 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/enskit-react-example/src/RegistryView.tsx` at line 86, The JSX is rendering a literal "$" because the id value is inside plain text instead of a JS expression; update the rendering in RegistryView so you don't use a template-string-looking literal. Replace occurrences like the text containing "id: ${byIdResult.data?.registry?.id ?? '-'}" and the similar one for byNameResult at Line 102 by removing the "$" and moving the expression into JSX curly braces (e.g., render the static label "id:" and then {byIdResult.data?.registry?.id ?? "-"}, similarly for byNameResult.data?.registry?.id) so the actual value is displayed instead of the literal dollar sign.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enskit/biome.jsonc`:
- Around line 7-12: The global linter override disabling
"noUnusedFunctionParameters" should be removed from the biome.jsonc linter.rules
block; instead, revert that rule to its default and locally suppress or rename
unused parameters only where necessary in
src/react/omnigraph/lib/cache-exchange.ts (e.g., add // eslint-disable-next-line
`@typescript-eslint/no-unused-vars` or rename resolver parameters to _unused or
_args on the specific resolver functions that require extra args). Update only
the linter config to remove the broad "noUnusedFunctionParameters": "off" entry
and modify the resolver function signatures or add per-line/local suppressions
around the specific functions in cache-exchange.ts.
In `@packages/enskit/src/react/omnigraph/lib/cache-exchange.ts`:
- Around line 65-109: The cache resolvers (account, registry, resolver,
permissions) currently assert args.by and throw Error("never") on unexpected
input; change each to first guard that args.by exists (e.g., const by = args.by
as ...; if (!by) return passthrough(args, cache, info)), then only return the {
__typename, id: ... } objects when the respective fields (by.id, by.address,
by.contract) are present, and otherwise fall back to passthrough(args, cache,
info) instead of throwing; retain use of helpers makeRegistryId, makeResolverId,
and makePermissionsId when constructing IDs.
---
Duplicate comments:
In `@examples/enskit-react-example/src/main.tsx`:
- Around line 27-30: Replace the plain anchor tags inside the <nav> with
client-side router links so internal navigation to /domain/eth, /registry and
/pagination uses the app router (e.g., react-router-dom's <Link to="...">)
instead of full-page reloads; import the router Link component at the top of
main.tsx, swap the internal <a href="..."> elements for <Link to="..."> inside
the nav, and leave any external links as <a> if present, so
OmnigraphProvider/URQL in-memory cache is preserved during navigation.
In `@examples/enskit-react-example/src/RegistryView.tsx`:
- Around line 25-29: The CacheStatus component currently treats cacheOutcome as
truthy/falsy which mislabels "miss" and "partial" as success; update CacheStatus
to read result.operation.context.meta.cacheOutcome explicitly and switch on its
possible values (e.g., "hit", "miss", "partial", and a fallback like
undefined/"unknown") so each maps to the correct label/color (e.g., green for
"hit", orange for "partial", red or orange for "miss", and a neutral label for
undefined). Locate the CacheStatus function and replace the current truthy check
with an explicit switch/if-else over cacheOutcome to render accurate status text
and color for each known outcome.
- Line 86: The JSX is rendering a literal "$" because the id value is inside
plain text instead of a JS expression; update the rendering in RegistryView so
you don't use a template-string-looking literal. Replace occurrences like the
text containing "id: ${byIdResult.data?.registry?.id ?? '-'}" and the similar
one for byNameResult at Line 102 by removing the "$" and moving the expression
into JSX curly braces (e.g., render the static label "id:" and then
{byIdResult.data?.registry?.id ?? "-"}, similarly for
byNameResult.data?.registry?.id) so the actual value is displayed instead of the
literal dollar sign.
In `@packages/enskit/src/react/omnigraph/lib/cache-exchange.ts`:
- Around line 22-27: Remove the incomplete JSDoc block above the passthrough
function; delete the multi-line comment that includes the unfinished description
and the redundant `@returns` tag so the function declaration `const passthrough
= (args: Variables, cache: Cache, info: ResolveInfo) =>` stands without the
misleading JSDoc (optionally replace it with a single-line comment like
"passthrough resolver" if a brief note is desired).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 05056870-0fa5-4c17-9da3-3c80569b1bcf
📒 Files selected for processing (8)
examples/enskit-react-example/package.jsonexamples/enskit-react-example/src/PaginationView.tsxexamples/enskit-react-example/src/RegistryView.tsxexamples/enskit-react-example/src/main.tsxpackages/enskit/biome.jsoncpackages/enskit/src/react/omnigraph/client.tspackages/enskit/src/react/omnigraph/lib/cache-exchange.tspackages/enskit/src/react/omnigraph/lib/local-connection-resolvers.ts
- fix literal $ in JSX and CacheStatus truthy/falsy bug in RegistryView - use Link instead of anchor tags to preserve SPA state in example nav - complete incomplete JSDoc on passthrough resolver - fix misleading CAIP-19 JSDoc on uint256ToHex32 - fix React.ReactNode namespace reference in OmnigraphProvider - add response.ok guard in omnigraph query module Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/enssdk/src/omnigraph/module.ts`:
- Around line 55-59: Add a unit test that verifies the error branch when
response.ok is false: mock fetch to return { ok: false, status: 400, statusText:
'Bad Request' }, create the test client with createMockClient(mockFetch), call
client.omnigraph.query(...) and assert it rejects with the message "Omnigraph
query failed: 400 Bad Request" so the guard around response.ok and the thrown
Error in the module.ts path are covered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c65b4520-b7de-4212-b25f-6848589e6b41
📒 Files selected for processing (8)
apps/ensapi/src/omnigraph-api/schema/permissions.integration.test.tsapps/ensapi/src/test/integration/find-events/event-pagination-queries.tsexamples/enskit-react-example/src/RegistryView.tsxexamples/enskit-react-example/src/main.tsxpackages/enskit/src/react/omnigraph/lib/cache-exchange.tspackages/enskit/src/react/omnigraph/provider.tsxpackages/enssdk/src/lib/helpers.tspackages/enssdk/src/omnigraph/module.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 104 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
apps/ensindexer/src/plugins/tokenscope/lib/handle-nft-transfer.ts:48
- Switching the NameToken primary key from the previous
formatAssetId(...)representation tostringifyAssetId(...)will change the persistedensIndexerSchema.nameTokens.idvalues (old format used a zero-padded 32-byte hex tokenId; new CAIP string usestokenId.toString()), so existing records won’t be found/updated and new duplicates may be inserted. This likely needs an explicit DB migration/backfill (or a compatibility layer that can read both formats) before using the new key format in production.
apps/ensindexer/src/plugins/tokenscope/handlers/Seaport.ts:58 - Using
stringifyAssetId(sale.nft)here changes the persistedassetIdstring format (tokenId becomes decimal viatoString()instead of the previous 32-byte hex encoding). IfensIndexerSchema.nameSales.assetId(or downstream consumers) rely on the legacy formatting, this will fragment historical data and break lookups/joins unless a migration/backfill or dual-format handling is added.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/enssdk/src/omnigraph/module.test.ts (1)
14-97: 🧹 Nitpick | 🔵 TrivialConsider adding a test for the error path when
response.okis false.The implementation throws when
response.okis false, but no test verifies this behavior. Adding coverage would ensure the error handling remains correct.💡 Suggested test case
it("throws when response is not ok", async () => { const mockFetch = vi.fn().mockResolvedValue({ ok: false, status: 500, statusText: "Internal Server Error", }); const client = createMockClient(mockFetch); await expect( client.omnigraph.query({ query: "query { domains { name } }" }), ).rejects.toThrow("Omnigraph query failed: 500 Internal Server Error"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/enssdk/src/omnigraph/module.test.ts` around lines 14 - 97, Add a test that asserts client.omnigraph.query throws when the fetch response has ok: false by creating a mocked fetch (mockFetch) that resolves to { ok: false, status: 500, statusText: "Internal Server Error" }, use createMockClient(mockFetch) to build the client, then await expect(client.omnigraph.query({ query: "query { domains { name } }" })).rejects.toThrow(...) to verify the error message includes the HTTP status and statusText (e.g., "Omnigraph query failed: 500 Internal Server Error").
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/enssdk/src/omnigraph/module.test.ts`:
- Around line 14-97: Add a test that asserts client.omnigraph.query throws when
the fetch response has ok: false by creating a mocked fetch (mockFetch) that
resolves to { ok: false, status: 500, statusText: "Internal Server Error" }, use
createMockClient(mockFetch) to build the client, then await
expect(client.omnigraph.query({ query: "query { domains { name } }"
})).rejects.toThrow(...) to verify the error message includes the HTTP status
and statusText (e.g., "Omnigraph query failed: 500 Internal Server Error").
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cd5a3a4-2397-45a3-94bf-37890469ae2e
📒 Files selected for processing (3)
packages/enskit/src/react/omnigraph/client.tspackages/enskit/src/react/omnigraph/provider.tsxpackages/enssdk/src/omnigraph/module.test.ts
- read response body on non-2xx and include in thrown error for diagnosability - add test covering the error path - pass EnsNodeClientConfig.fetch through to urql Client Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 99 out of 104 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Invariant: subregistry record must exist | ||
| if (!subregistry) { | ||
| throw new Error(`Subregistry record must exists for '${formatAccountId(subregistryId)}.'`); | ||
| throw new Error(`Subregistry record must exists for '${stringifyAccountId(subregistryId)}.'`); |
There was a problem hiding this comment.
Grammar: error message says "must exists"; should be "must exist" (and consider moving the trailing period outside the quoted subregistry id).
| // Invariant: subregistry record must exist | ||
| if (!subregistry) { | ||
| throw new Error(`Subregistry record must exists for '${formatAccountId(subregistryId)}.'`); | ||
| throw new Error(`Subregistry record must exists for '${stringifyAccountId(subregistryId)}.'`); |
There was a problem hiding this comment.
Grammar: error message says "must exists"; should be "must exist" (and consider moving the trailing period outside the quoted subregistry id).
Comment on lines
+88
to
+92
| if (by.id) return { __typename: "Registry", id: by.id }; | ||
| if (by.contract) return { __typename: "Registry", id: makeRegistryId(by.contract) }; | ||
|
|
||
| throw new Error("never"); | ||
| }, |
There was a problem hiding this comment.
Throwing new Error("never") makes failures hard to debug when the resolver is called with unexpected args. Please replace this with a descriptive message (e.g., indicating args.by must contain exactly one of the allowed fields) or assert/unreachable helper that prints the received args.
Comment on lines
+96
to
+100
| if (by.id) return { __typename: "Resolver", id: by.id }; | ||
| if (by.contract) return { __typename: "Resolver", id: makeResolverId(by.contract) }; | ||
|
|
||
| throw new Error("never"); | ||
| }, |
There was a problem hiding this comment.
Throwing new Error("never") makes failures hard to debug when the resolver is called with unexpected args. Please replace this with a descriptive message (e.g., indicating args.by must contain exactly one of the allowed fields) or assert/unreachable helper that prints the received args.
Comment on lines
+104
to
+108
| if (by.id) return { __typename: "Permissions", id: by.id }; | ||
| if (by.contract) return { __typename: "Permissions", id: makePermissionsId(by.contract) }; | ||
|
|
||
| throw new Error("never"); | ||
| }, |
There was a problem hiding this comment.
Throwing new Error("never") makes failures hard to debug when the resolver is called with unexpected args. Please replace this with a descriptive message (e.g., indicating args.by must contain exactly one of the allowed fields) or assert/unreachable helper that prints the received args.
shrugs
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #1796
references #1793
Reviewer Focus
packages/enskit/src/react/omnigraph/— new package surface area. graphcache configuration (cache-exchange, local-connection-resolvers) is where most of the complexity lives.packages/enssdk/src/lib/ids.tsandpackages/enssdk/src/lib/caip.ts— id construction and CAIP serialization moved here from ensnode-sdk. verify nothing was lost or changed in behavior.Query.accountandQuery.permissionsnow usebyinput pattern.Problem & Motivation
makeRegistryId,makeENSv1DomainId, etc.) and CAIP serialization (formatAccountId,formatAssetId) lived in ensnode-sdk, but they belong in the lower-level enssdk so both enskit and ensindexer can depend on them without pulling in all of ensnode-sdk.Query.accountandQuery.permissionsdidn't follow theby: { id, ... }input pattern already used byQuery.registryandQuery.resolver, making the API inconsistent and graphcache resolution harder.What Changed (Concrete)
enskitpackage (packages/enskit/src/react/omnigraph/) — urql-basedOmnigraphProvider,useOmnigraphQueryhook, graphcache with schema-driven relay pagination and entity-aware local resolversensnode-sdk/ensv2/ids-libtoenssdk/lib/idsensnode-sdk/shared/serializetoenssdk/lib/caip, renamingformatAccountId→stringifyAccountId,formatAssetId→stringifyAssetIdinterpretTokenIdAsLabelHashandinterpretTokenIdAsNodefromensnode-sdktoenssdk/lib/interpretationuint256ToHex32fromensnode-sdk/ens/subname-helperstoenssdk/lib/helpersgetStorageId→makeStorageIdfor consistency with othermake*constructorsQuery.accountnow acceptsby: { id, address },Query.permissionsacceptsby: { id, contract }— matching existingbypatternDefaultEdgesNullability: false,DefaultNodeNullability: false)enssdkfor moved itemsenskit-react-exampleapp demonstrating domain lookup, registry browsing, and paginationenssdk/omnigraph/index.tsinto separatemodule.tsfile, added integration testDesign & Planning
enskit wraps urql + graphcache rather than building a custom cache, giving us normalized caching and relay pagination with minimal code
localConnectionResolversderives relay pagination resolvers from the introspection schema so they don't need manual maintenance as the schema evolvesid construction logic was moved to enssdk because it's the shared foundation — enskit, ensindexer, and ensnode-sdk all need it
Planning artifacts: Narrative Overhaul: Client Packages #1793
Reviewed / approved by: n/a
Self-Review
enssdk/omnigraph/index.tsfrom inline module to separatemodule.tsformatAccountId/formatAssetId→stringifyAccountId/stringifyAssetId,getStorageId→makeStorageIdensnode-sdk/ensv2/directory,ensnode-sdk/shared/interpretation/interpret-tokenid.ts, removed re-exportsCross-Codebase Alignment
formatAccountId,formatAssetId,getStorageId,interpretTokenId,uint256ToHex32packages/ensnode-sdk/src/tokenscope/zod-schemas.ts(uses AssetId types but doesn't need the moved functions)Downstream & Consumer Impact
consumers of ensnode-sdk who used
formatAccountId,formatAssetId,getStorageId, orinterpretTokenId*will need to import fromenssdkinsteadQuery.account(address:)→Query.account(by: { address: })andQuery.permissions(for:)→Query.permissions(by: { contract: })are breaking GraphQL changesconnection edges and nodes are now non-nullable, which is a schema change visible to all omnigraph consumers
Public APIs affected: omnigraph GraphQL schema, ensnode-sdk exports (removed), enssdk exports (added)
Docs updated: example queries updated to use new
bypatternNaming decisions worth calling out:
stringify*overformat*to align with JS conventions (JSON.stringify,URL.toString)Testing Evidence
Query.*field — these are runtime cache behaviors that are hard to unit test without a full urql test clientScope Reductions
Risk Analysis
byargument destructuring doesn't match the actual schema inputs; breaking GraphQL schema changes for existing omnigraph consumersPre-Review Checklist (Blocking)