Build ENSRainbow config #1425
Build ENSRainbow config #1425
Conversation
…NSRainbow application
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: aeb14c9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
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:
📝 WalkthroughWalkthroughCentralizes ENSRainbow configuration with Zod schemas and a runtime Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant SDK as "SDK Client"
participant Server as "ENSRainbow API"
participant DB as "Database"
participant Builder as "buildENSRainbowPublicConfig"
SDK->>Server: GET /v1/config
Server->>DB: getServerLabelSet() (cached)
Server->>DB: labelCount() (startup-cached)
DB-->>Server: labelSet, count
Server->>Builder: assemble public config (version, labelSet, recordsCount)
Builder-->>Server: ENSRainbowPublicConfig
Server-->>SDK: 200 + ENSRainbowPublicConfig
sequenceDiagram
autonumber
participant CLI as "CLI"
participant ConfigBuilder as "buildConfigFromEnvironment"
participant Zod as "Zod schemas"
participant Process as "process.env"
CLI->>ConfigBuilder: request config from env
ConfigBuilder->>Zod: parse & validate environment
Zod-->>ConfigBuilder: validated ENSRainbowConfig or errors
alt valid
ConfigBuilder-->>CLI: return ENSRainbowConfig
CLI->>CLI: proceed with startup using config (serve/ingest)
else invalid
ConfigBuilder->>Process: log errors (pretty)
Process->>Process: exit non-zero
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@apps/ensrainbow/src/config/config.schema.ts`:
- Around line 65-71: The current ternary for labelSet uses a truthy check
(env.LABEL_SET_ID || env.LABEL_SET_VERSION) which treats empty strings as
missing; change the condition to explicit undefined checks so an empty string is
treated as a provided value and validation will run — e.g. replace the condition
with (env.LABEL_SET_ID !== undefined || env.LABEL_SET_VERSION !== undefined) and
still return the object with labelSetId: env.LABEL_SET_ID and labelSetVersion:
env.LABEL_SET_VERSION when true; keep the symbol name labelSet and the env keys
env.LABEL_SET_ID / env.LABEL_SET_VERSION so locators remain obvious.
- Around line 33-36: The schema currently calls getDefaultDataDir() at module
load in ENSRainbowConfigSchema (dataDir:
DataDirSchema.default(getDefaultDataDir())), capturing process.cwd() too early;
remove the eager default from ENSRainbowConfigSchema and instead handle lazy
evaluation in buildConfigFromEnvironment by supplying dataDir: env.DATA_DIR ??
getDefaultDataDir() when parsing/building the config, keeping
ENSRainbowConfigSchema (and DataDirSchema/PortSchema) purely declarative and
ensuring getDefaultDataDir() runs only at build time.
- Around line 18-24: The path transform in the config schema currently treats
paths starting with "/" as absolute; update the transform used on the config
field to use Node's path.isAbsolute(path) instead of path.startsWith("/"), and
ensure the Node "path" module is imported (or isAbsolute is referenced)
alongside the existing join and process.cwd() usage in the transform callback so
Windows absolute paths like "C:\..." are detected correctly and returned
unchanged.
- Around line 73-83: Replace the terminal process.exit(1) in the catch block
with throwing a descriptive error so callers can handle failures; specifically,
inside the catch for buildConfigFromEnvironment (or whatever function constructs
ENSRainbowConfig) throw a custom error (e.g., ConfigBuildError) or rethrow the
existing Error with context including the prettified ZodError output and the
message "Failed to build ENSRainbowConfig", while keeping the existing logger
calls for ZodError and generic Error; move any process.exit(1) behavior out to
the CLI/entrypoint so tests can catch the thrown error and decide whether to
exit.
In `@apps/ensrainbow/src/config/validations.ts`:
- Around line 7-10: The current type ZodCheckFnInput<T> uses the internal
z.core.ParsePayload<T>; change it to rely on Zod's documented types or a simple
explicit input shape instead: remove z.core.ParsePayload and either use the
public helper z.input with a Zod type (e.g., z.input<z.ZodType<T>>) or replace
ZodCheckFnInput<T> with a small explicit interface/alias (e.g., unknown or
Record<string, any> or a narrow shape your check expects) so the code no longer
depends on the unstable z.core namespace; update any usages of ZodCheckFnInput
to match the new public type.
In `@apps/ensrainbow/src/lib/env.ts`:
- Around line 7-10: The getEnvPort function unsafely asserts process.env as
ENSRainbowEnvironment and rebuilds the full config on every call; remove the
type assertion and instead import the ENSRainbowConfig type (import type {
ENSRainbowConfig } ...) and let buildConfigFromEnvironment validate process.env
at runtime, receiving an ENSRainbowConfig result; then read and return
config.port. Also memoize the built config in a module-level variable so
getEnvPort calls reuse the same config instead of reconstructing it each time
(references: getEnvPort, buildConfigFromEnvironment, ENSRainbowEnvironment,
ENSRainbowConfig).
There was a problem hiding this comment.
Pull request overview
Introduces a Zod-based, centralized environment configuration builder for the ENSRainbow app, aligning it with the configuration patterns used in other apps in the monorepo.
Changes:
- Added ENSRainbow config schema, environment types, defaults, and cross-field validations.
- Updated ENSRainbow CLI/env port handling to use the new config builder and centralized defaults.
- Tightened shared
PortSchemavalidation to require integer ports; addedzodas a direct ENSRainbow dependency.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds zod to the ENSRainbow importer lock entry. |
| packages/ensnode-sdk/src/shared/config/zod-schemas.ts | Updates shared PortSchema to require integer ports. |
| apps/ensrainbow/src/lib/env.ts | Switches env port resolution to buildConfigFromEnvironment(...). |
| apps/ensrainbow/src/config/validations.ts | Adds ENSRainbow-specific invariant validation for schema version. |
| apps/ensrainbow/src/config/types.ts | Re-exports ENSRainbow config type. |
| apps/ensrainbow/src/config/index.ts | Adds a config module entrypoint exporting types/functions/defaults. |
| apps/ensrainbow/src/config/environment.ts | Defines typed raw environment shape for ENSRainbow. |
| apps/ensrainbow/src/config/defaults.ts | Centralizes ENSRainbow default port and data dir. |
| apps/ensrainbow/src/config/config.schema.ts | Adds ENSRainbow Zod schema + config builder with logging/exit-on-failure behavior. |
| apps/ensrainbow/src/cli.ts | Uses new defaults module for data dir default; continues using env-derived port. |
| apps/ensrainbow/src/cli.test.ts | Updates port tests to reflect process-exit behavior on invalid PORT values. |
| apps/ensrainbow/package.json | Adds zod as an explicit dependency for ENSRainbow. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
… for environment variable handling
…d of environment variable
… and update imports accordingly
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 22 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.
| coerce: (port: number) => { | ||
| // Validate port using PortSchema (make it required by parsing with a non-optional schema) | ||
| const result = PortSchema.safeParse(port); | ||
| if (!result.success) { | ||
| const firstError = result.error.issues[0]; | ||
| throw new Error(`Invalid port: ${firstError?.message ?? "invalid port number"}`); | ||
| } | ||
| if (result.data === undefined) { | ||
| throw new Error("Invalid port: port is required"); | ||
| } | ||
| return result.data; | ||
| }, |
There was a problem hiding this comment.
The coerce handler is validating with PortSchema, but PortSchema is explicitly optional. This makes the extra result.data === undefined branch necessary and contradicts the comment about using a non-optional schema. Prefer validating with the non-optional PortSchemaBase (or a .required()/non-optional variant) so the return type is always a number and the undefined branch + comment mismatch can be removed.
| } catch (_error) { | ||
| logger.error("Cannot start server: database is empty or uninitialized."); | ||
| logger.error("The database must contain ingested data before the server can start."); | ||
| logger.error("Please run the ingestion command first to populate the database."); | ||
| // Throw error to ensure process exits with non-zero status code for CI/CD and scripts | ||
| throw new Error("Database is empty or uninitialized. Cannot start server."); |
There was a problem hiding this comment.
The getPrecalculatedRainbowRecordCount() pre-check treats any thrown error as "database is empty or uninitialized" and discards the underlying error context. This can misdiagnose cases like a corrupted/invalid precalculated count value or LevelDB read errors. Consider either (a) including the original error message in the thrown/logged output, or (b) only mapping the specific "no precalculated count" case to the "empty/uninitialized" guidance and letting other errors propagate.
| } catch (_error) { | |
| logger.error("Cannot start server: database is empty or uninitialized."); | |
| logger.error("The database must contain ingested data before the server can start."); | |
| logger.error("Please run the ingestion command first to populate the database."); | |
| // Throw error to ensure process exits with non-zero status code for CI/CD and scripts | |
| throw new Error("Database is empty or uninitialized. Cannot start server."); | |
| } catch (error) { | |
| // Log the underlying error so issues like corruption or read failures are visible | |
| logger.error(error, "Error while checking precalculated rainbow record count"); | |
| logger.error("Cannot start server: database is empty or uninitialized."); | |
| logger.error("The database must contain ingested data before the server can start."); | |
| logger.error("Please run the ingestion command first to populate the database."); | |
| // Throw error to ensure process exits with non-zero status code for CI/CD and scripts | |
| const originalMessage = | |
| error instanceof Error ? error.message : String(error); | |
| throw new Error( | |
| `Database is empty or uninitialized. Cannot start server. Underlying error: ${originalMessage}`, | |
| ); |
…ve logging in server command
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 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.
| LABEL_SET_ID?: string; | ||
| LABEL_SET_VERSION?: string; |
There was a problem hiding this comment.
LABEL_SET_ID / LABEL_SET_VERSION are declared as supported env vars but are not consumed by buildConfigFromEnvironment (and appear unused elsewhere). Either wire them into the config schema or remove them to avoid implying unsupported configuration knobs.
| LABEL_SET_ID?: string; | |
| LABEL_SET_VERSION?: string; |
| const errorData = (await response.json()) as { error?: string; errorCode?: number }; | ||
| throw new Error( | ||
| errorData.error ?? `Failed to fetch ENSRainbow config: ${response.statusText}`, | ||
| ); |
There was a problem hiding this comment.
config() throws on non-2xx responses after unconditionally calling response.json(). If the server returns a non-JSON body for errors (e.g., 404 HTML/plain text), this will throw a JSON parse error and hide the real HTTP failure. Consider wrapping JSON parsing in try/catch and falling back to response.text() (and include status/statusText) when the body isn’t JSON.
| const errorData = (await response.json()) as { error?: string; errorCode?: number }; | |
| throw new Error( | |
| errorData.error ?? `Failed to fetch ENSRainbow config: ${response.statusText}`, | |
| ); | |
| let errorMessage = `Failed to fetch ENSRainbow config: ${response.status} ${response.statusText}`; | |
| try { | |
| const bodyText = await response.text(); | |
| if (bodyText) { | |
| try { | |
| const errorData = JSON.parse(bodyText) as { error?: string; errorCode?: number }; | |
| if (errorData && errorData.error) { | |
| errorMessage = errorData.error; | |
| } else { | |
| errorMessage = `${errorMessage} - ${bodyText}`; | |
| } | |
| } catch { | |
| // Body is not valid JSON; include raw text in the message. | |
| errorMessage = `${errorMessage} - ${bodyText}`; | |
| } | |
| } | |
| } catch { | |
| // If reading the body fails, fall back to the default message. | |
| } | |
| throw new Error(errorMessage); |
…ecific database error messages
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 21 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.
| import config from "@/config"; | ||
|
|
There was a problem hiding this comment.
Importing the default config at module load means env validation runs before yargs parses CLI args. If a user has an invalid PORT/DATA_DIR/DB_SCHEMA_VERSION in the environment, the CLI will exit even when --port/--data-dir would otherwise override those values, which contradicts the “CLI arguments take precedence” note. Consider building the env config lazily (after argv parsing) or validating only the subset of env vars that are actually used as defaults for the chosen command.
| // Check if the database is empty (no precalculated count) | ||
| // This prevents starting a server that can't serve any data | ||
| try { | ||
| await db.getPrecalculatedRainbowRecordCount(); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if (message.includes("No precalculated count found")) { | ||
| logger.error("Cannot start server: database is empty or uninitialized."); | ||
| logger.error("The database must contain ingested data before the server can start."); | ||
| logger.error("Please run the ingestion command first to populate the database."); | ||
| throw new Error("Database is empty or uninitialized. Cannot start server."); | ||
| } |
There was a problem hiding this comment.
This startup check relies on substring-matching the thrown error message ("No precalculated count found...") to detect an empty/uninitialized DB. That’s brittle: changing the message in getPrecalculatedRainbowRecordCount() would silently change server behavior. Prefer a structured signal (e.g., return null/Result type, throw a dedicated error class, or expose a boolean hasPrecalculatedRainbowRecordCount() helper) and branch on that instead of parsing strings.
| // Check if the database is empty (no precalculated count) | |
| // This prevents starting a server that can't serve any data | |
| try { | |
| await db.getPrecalculatedRainbowRecordCount(); | |
| } catch (error) { | |
| const message = error instanceof Error ? error.message : String(error); | |
| if (message.includes("No precalculated count found")) { | |
| logger.error("Cannot start server: database is empty or uninitialized."); | |
| logger.error("The database must contain ingested data before the server can start."); | |
| logger.error("Please run the ingestion command first to populate the database."); | |
| throw new Error("Database is empty or uninitialized. Cannot start server."); | |
| } | |
| // Check if the database has a precalculated count. | |
| // This prevents starting a server that can't serve any data. | |
| try { | |
| await db.getPrecalculatedRainbowRecordCount(); | |
| } catch (error) { |
| // Check if the database is empty (no precalculated count) | ||
| // This prevents starting a server that can't serve any data | ||
| try { | ||
| await db.getPrecalculatedRainbowRecordCount(); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| if (message.includes("No precalculated count found")) { | ||
| logger.error("Cannot start server: database is empty or uninitialized."); | ||
| logger.error("The database must contain ingested data before the server can start."); | ||
| logger.error("Please run the ingestion command first to populate the database."); | ||
| throw new Error("Database is empty or uninitialized. Cannot start server."); | ||
| } |
There was a problem hiding this comment.
This PR changes server startup behavior to refuse to start when the database has no precalculated record count (treated as empty/uninitialized), but the PR description focuses on config + the new /v1/config endpoint. Please document this behavioral change (and its rationale) in the PR description or release notes, since it can break existing deployments that previously started with an empty DB.
| @@ -1,4 +1,5 @@ | |||
| import packageJson from "@/../package.json"; | |||
There was a problem hiding this comment.
In ESM, importing JSON without an import attribute can throw at runtime (ERR_IMPORT_ASSERTION_TYPE_MISSING) depending on the loader/bundler. Elsewhere in the repo the pattern is import packageJson ... with { type: "json" }. For consistency and to avoid runtime differences between tsx/vitest/node, update this import to use the same JSON import attribute form.
| import packageJson from "@/../package.json"; | |
| import packageJson from "@/../package.json" with { type: "json" }; |
|
There is a problem with current solution and also layered Configs (EnvConfig -> ArgsConfig). If env value is invalid but arg value is valid then the validation process will fail on building EnvConfig but it should pass IMO. |
3 participants
Related to #1407
Lite PR
Summary
/v1/configendpoint returning public config (version, label set, records count) and deprecated/v1/versionWhy
Testing
config.schema.test.ts) covering success cases, validation errors, invariants, and edge cases/v1/configendpoint in server command testsNotes for Reviewer (Optional)
/v1/versionendpoint is deprecated but still functional for backward compatibilityPre-Review Checklist (Blocking)