Conversation
🦋 Changeset detectedLatest commit: f608c31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
@greptile review |
|
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:
📝 WalkthroughWalkthroughAdds ENSDb persistence and a background writer: Drizzle factory, ENSDb client and singleton, writer worker + starter singleton, indexer/indexing-status singletons, tests/mocks, package dependency additions, and updates the API handler to use singletons and start the writer. Changes
Sequence DiagramsequenceDiagram
participant API as "ensnode-api"
participant Worker as "EnsDbWriterWorker (singleton)"
participant Indexer as "IndexingStatusBuilder / EnsIndexerClient"
participant Client as "EnsDbClient"
participant DB as "PostgreSQL"
API->>Worker: startEnsDbWriterWorker()
rect rgba(100,150,255,0.5)
Note over Worker,Indexer: initial validation & config fetch
Worker->>Indexer: getValidatedEnsIndexerPublicConfig()
Indexer->>Client: request stored config
Client->>DB: SELECT metadata by key
DB-->>Client: stored config or none
Client-->>Worker: config (or undefined)
end
rect rgba(100,150,255,0.5)
Note over Worker: initial upserts
Worker->>Client: upsertEnsDbVersion()
Client->>DB: upsert version metadata
Worker->>Client: upsertEnsIndexerPublicConfig()
Client->>DB: upsert config metadata
end
rect rgba(150,200,100,0.5)
Note over Worker: streaming snapshots
loop every interval
Worker->>Indexer: buildIndexingStatusSnapshot()
Indexer-->>Worker: snapshot
Worker->>Client: upsertIndexingStatusSnapshot()
Client->>DB: upsert serialized snapshot
DB-->>Client: OK
end
end
alt Worker stops normally
Worker-->>API: stopped
else Unrecoverable error / retries exhausted
Worker-->>API: error -> process abort
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Introduces an ENSDb “writer worker” in ENSIndexer to persist ENSIndexer metadata into ENSDb, and updates the ensnode-sdk ENSDb client interface naming to be explicitly ENSIndexer-scoped.
Changes:
- Add
EnsDbClient(Drizzle-based) for reading/writing ENSNode metadata records in ENSDb, plus unit tests/mocks. - Add
EnsDbWriterWorkerthat upserts ENSDb version + ENSIndexer public config once, then periodically upserts indexing status snapshots (with retries), plus unit tests. - Wire the worker + new singletons into ENSIndexer’s Hono API handler; add required deps (
drizzle-orm,@ponder/client,@types/pg).
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds/threads @types/pg through relevant dependency snapshots. |
| packages/ensnode-sdk/src/ensdb/client.ts | Renames ENSDb client query/mutation methods to ENSIndexer-specific names. |
| apps/ensindexer/src/lib/indexing-status-builder/singleton.ts | Introduces singleton instance for IndexingStatusBuilder. |
| apps/ensindexer/src/lib/ensindexer-client/singleton.ts | Introduces singleton EnsIndexerClient configured via app config. |
| apps/ensindexer/src/lib/ensdb-writer-worker/singleton.ts | Creates a singleton EnsDbWriterWorker wired with required dependencies. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts | Implements the worker loop, validation, and retry behavior. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts | Adds unit tests for worker behavior (currently has signature mismatches). |
| apps/ensindexer/src/lib/ensdb-client/singleton.ts | Adds singleton EnsDbClient for ENSIndexer. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts | Implements ENSDb client queries/mutations for ENSNode metadata. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.test.ts | Adds unit tests for ENSDb client behavior. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.mock.ts | Adds fixtures for ENSDb client tests (public config, serialized snapshot). |
| apps/ensindexer/src/lib/ensdb-client/drizzle.ts | Adds makeDrizzle helper (adapted from ENSApi). |
| apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts | Starts the writer worker on module load and swaps in singleton builder usage. |
| apps/ensindexer/package.json | Adds needed runtime/dev deps for Drizzle and pg typings. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/ensindexer/package.json`:
- Line 49: Update the dependency entry for "@types/pg" in package.json to use
the workspace catalog reference (matching how "@types/node" is declared) instead
of the hardcoded "8.16.0"; if "@types/pg" is not yet listed in the pnpm
workspace catalog, add it there with the desired version and then change the
package.json entry to "catalog:`@types/pg`" (or the exact catalog key used in your
monorepo) so the monorepo versioning pattern remains consistent.
In `@apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts`:
- Around line 24-32: Replace the current un-awaited promise handling for
ensDbWriterWorker.runWithRetries so it doesn't create an unhandled rejection:
either move the call into a ponder.on("setup", ...) handler and await
ensDbWriterWorker.runWithRetries({ maxRetries: 3 }) so failures can be thrown to
fail setup, or keep it fire-and-forget but mark failure deterministically by
using void ensDbWriterWorker.runWithRetries(...).catch(...) and inside the catch
call ensDbWriterWorker.stop(), log the error and set process.exitCode = 1;
update the code around ensDbWriterWorker.runWithRetries and related stop/logging
accordingly.
In `@apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts`:
- Around line 131-136: Remove the redundant `@returns` JSDoc tag from the JSDoc
block that begins "Get ENSNode metadata record" in ensdb-client.ts (the doc for
the ENS node metadata retrieval method); keep the summary and other tags such as
`@throws` but delete the `@returns` line so the comment no longer restates the
method summary.
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts`:
- Line 242: The call to worker.runWithRetries currently passes a numeric
argument but the method signature expects an options object; change the
invocation of runWithRetries on the worker instance to pass an object like {
maxRetries: 2 } (mirror the same fix you applied at the earlier call near line
210) so the call matches the runWithRetries({ maxRetries: number }) signature.
- Line 210: The test calls worker.runWithRetries(2) but runWithRetries expects
an object parameter; change the call to pass an object like
worker.runWithRetries({ maxRetries: 2 }) (update the runPromise assignment and
any other test invocations of runWithRetries accordingly) so the signature for
runWithRetries and the worker variable usage match the expected { maxRetries:
number } shape.
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts`:
- Around line 158-160: Remove the redundant JSDoc `@returns` lines from the method
docblocks that describe returning the in-memory config object (the block that
currently says "In-memory config object, if the validation is successful or if
there is no stored config.") as well as the similar docblock around lines
204-207; keep the summary and `@throws` entries but delete the repetitive `@returns`
tags so the method documentation follows repo style.
- Around line 113-136: The retry loop in runWithRetries is unbounded because it
loops while (!this.isStopped) and only checks maxRetries after sleeping, which
can cause runaway timers; change the loop to explicitly bound retries (e.g.,
while (attempt < maxRetries && !this.isStopped)) or otherwise include an attempt
check in the loop condition so the delay on Line 130 cannot execute beyond
maxRetries; keep the existing behavior of throwing an Error with cause when
attempts are exhausted (the throw that references attempt and error should
remain inside the failure branch) and continue using
INDEXING_STATUS_RECORD_UPDATE_INTERVAL for the sleep duration.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/ensindexer/package.jsonapps/ensindexer/ponder/src/api/handlers/ensnode-api.tsapps/ensindexer/src/lib/ensdb-client/drizzle.tsapps/ensindexer/src/lib/ensdb-client/ensdb-client.mock.tsapps/ensindexer/src/lib/ensdb-client/ensdb-client.test.tsapps/ensindexer/src/lib/ensdb-client/ensdb-client.tsapps/ensindexer/src/lib/ensdb-client/singleton.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.tsapps/ensindexer/src/lib/ensdb-writer-worker/singleton.tsapps/ensindexer/src/lib/ensindexer-client/singleton.tsapps/ensindexer/src/lib/indexing-status-builder/singleton.tspackages/ensnode-sdk/src/ensdb/client.ts
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR introduces Key changes:
Two style observations remain: the guard error message in Confidence Score: 4/5
Last reviewed commit: 09d2b8e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts
Outdated
Show resolved
Hide resolved
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
c6ce77f to
328e5c1
Compare
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
328e5c1 to
c39e3ea
Compare
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
c39e3ea to
eed564c
Compare
ENSIndexer modules can import the singleton instance from `@/lib/ensdb-client`.
tk-o
force-pushed
the
feat/ensdb-writer-worker-for-ensindexer
branch
from
eed564c to
8208a39
Compare
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/ponder/src/api/ensdb-writer-worker-entrypoint.ts
Outdated
Show resolved
Hide resolved
…h call Uses `p-retry` to save ourselves from re-implementing it from scratch (like we tried in PR 1709).
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 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.
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 4 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.
|
@greptile review |
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 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.
apps/ensindexer/ponder/src/api/ensdb-writer-worker-entrypoint.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 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.
| it("calls pRetry for config fetch with retry logic", async () => { | ||
| // arrange - pRetry is mocked to call fn directly | ||
| const omnichainSnapshot = { | ||
| omnichainStatus: OmnichainIndexingStatusIds.Following, | ||
| omnichainIndexingCursor: 100, | ||
| chains: {}, | ||
| } as OmnichainIndexingStatusSnapshot; | ||
|
|
||
| const snapshot = { | ||
| strategy: CrossChainIndexingStrategyIds.Omnichain, | ||
| slowestChainIndexingCursor: 100, | ||
| snapshotTime: 200, | ||
| omnichainSnapshot, | ||
| } as CrossChainIndexingStatusSnapshot; | ||
|
|
||
| vi.mocked(buildCrossChainIndexingStatusSnapshotOmnichain).mockReturnValue(snapshot); | ||
|
|
||
| const ensDbClient = { | ||
| getEnsIndexerPublicConfig: vi.fn().mockResolvedValue(undefined), | ||
| upsertEnsDbVersion: vi.fn().mockResolvedValue(undefined), | ||
| upsertEnsIndexerPublicConfig: vi.fn().mockResolvedValue(undefined), | ||
| upsertIndexingStatusSnapshot: vi.fn().mockResolvedValue(undefined), | ||
| } as unknown as EnsDbClient; | ||
|
|
||
| const ensIndexerClient = { | ||
| config: vi.fn().mockResolvedValue(publicConfig), | ||
| } as unknown as EnsIndexerClient; | ||
|
|
||
| const indexingStatusBuilder = { | ||
| getOmnichainIndexingStatusSnapshot: vi.fn().mockResolvedValue(omnichainSnapshot), | ||
| } as unknown as IndexingStatusBuilder; | ||
|
|
||
| const worker = new EnsDbWriterWorker(ensDbClient, ensIndexerClient, indexingStatusBuilder); | ||
|
|
||
| // act | ||
| await worker.run(); | ||
|
|
||
| // assert - config should be called once (pRetry is mocked) | ||
| expect(ensIndexerClient.config).toHaveBeenCalledTimes(1); | ||
| expect(ensDbClient.upsertEnsIndexerPublicConfig).toHaveBeenCalledWith(publicConfig); |
There was a problem hiding this comment.
Test case "calls pRetry for config fetch with retry logic" doesn’t actually assert that pRetry was invoked or that the retry options were wired in; it only checks that ensIndexerClient.config ran once. To make this test meaningful, import pRetry in the test and assert it was called (and optionally that it received the expected retries/onFailedAttempt options).
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Nice updates 😄 Shared a few small comments. Please take the lead to merge when ready 👍
| @@ -0,0 +1,9 @@ | |||
| // This file is the entry point for the ENSDb Writer Worker. | |||
There was a problem hiding this comment.
Suggest to move all the ideas here (including this nice comment) into apps/ensindexer/ponder/src/api/index.ts
Goal: Not a fan of the import "./ensdb-writer-worker-entrypoint"; in that index.ts file because it's not so explicit what importing it does.
The call to startEnsDbWriterWorker can be in index.ts too which feels natural.
| // wait for the configured delay before the next attempt. | ||
| await this.nextTryDelay(); | ||
| } | ||
| console.log(`[EnsDbWriterWorker]: Upserting Indexing Status Snapshot into ENSDb...`); |
There was a problem hiding this comment.
Since we are running this task every second, perhaps best not to log this notices and instead only log error cases?
There was a problem hiding this comment.
Right, will drop these log entires. I wish Ponder allowed using its logger so we could log our application stuff and re-use Ponder's log levels: ponder-sh/ponder#1727
|
|
||
| resolve(); | ||
| }, secondsToMilliseconds(INDEXING_STATUS_RECORD_UPDATE_INTERVAL)); | ||
| // Invariant: the Omnichain Status must indicate that indexing has started already. |
There was a problem hiding this comment.
It will be great to document the why for this idea
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
EnsDbClientclass for ENSIndexer (includes queries and mutations).EnsDbWriterWorkerto handle tasks of writing ENSNode metadata into ENSDbstartEnsDbWriterWorkerfunction creates a singleton instance forEnsDbWriterWorkerand sets up error handling.Why
Testing
ENSIndexer logs demo
Notes for Reviewer (Optional)
EnsDbClientQueryinterface) from ENSDb that will be available via a new package so our customers could use it in their backend workloads. That update is out of scope for this PR, but when it's live, theapps/ensindexer/src/lib/ensdb-client/ensdb-client.tsimplementation could simply extend the "read-only" client and only add the mutation methods (seeEnsDbClientMutationinterface).Pre-Review Checklist (Blocking)
Resolves #1252