Build ENSRainbow config #1425
Open
Build ENSRainbow config #1425
Conversation
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
…NSRainbow application
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: e3a6c90 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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughCentralizes ENSRainbow configuration with Zod schemas and defaults, validates environment-to-config mapping (including DB schema invariant), moves CLI defaults to the new config, tightens port validation, adds a GET /v1/config endpoint and SDK client method, and expands tests for config and CLI isolation. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI"
participant ConfigBuilder as "buildConfigFromEnvironment"
participant Zod as "Zod schema"
participant Process as "process"
CLI->>ConfigBuilder: request config from env
ConfigBuilder->>Zod: parse & validate environment -> ENSRainbowConfig
Zod-->>ConfigBuilder: validation result
alt valid
ConfigBuilder-->>CLI: return ENSRainbowConfig
CLI->>CLI: continue startup (serve/ingest/validate)
else invalid
ConfigBuilder->>Process: log prettified errors
Process->>Process: process.exit(1)
end
sequenceDiagram
autonumber
participant SDK as "SDK Client"
participant Server as "ENSRainbow API"
participant DB as "Label store / DB"
participant Builder as "buildENSRainbowPublicConfig"
SDK->>Server: GET /v1/config
Server->>DB: fetch server label set & count
DB-->>Server: labelSet, count
Server->>Builder: assemble public config (config, labelSet, count)
Builder-->>Server: ENSRainbowPublicConfig
Server-->>SDK: 200 + ENSRainbowPublicConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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).
Comment on lines
+33
to
+36
| const ENSRainbowConfigSchema = z | ||
| .object({ | ||
| port: PortSchema.default(ENSRAINBOW_DEFAULT_PORT), | ||
| dataDir: DataDirSchema.default(getDefaultDataDir()), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Default data directory is evaluated at module load time.
getDefaultDataDir() is called once when the module loads, capturing process.cwd() at that moment. If the working directory changes before buildConfigFromEnvironment is called, the default will be stale.
Consider using a getter function for lazy evaluation:
♻️ Suggested lazy evaluation
const ENSRainbowConfigSchema = z
.object({
port: PortSchema.default(ENSRAINBOW_DEFAULT_PORT),
- dataDir: DataDirSchema.default(getDefaultDataDir()),
+ dataDir: DataDirSchema.optional(),
dbSchemaVersion: DbSchemaVersionSchema,
labelSet: LabelSetSchema.optional(),
})Then handle the default in buildConfigFromEnvironment:
return ENSRainbowConfigSchema.parse({
port: env.PORT,
dataDir: env.DATA_DIR ?? getDefaultDataDir(),
// ...
});🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 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.
Comment on lines
73
to
83
| } catch (error) { | ||
| if (error instanceof ZodError) { | ||
| logger.error(`Failed to parse environment configuration: \n${prettifyError(error)}\n`); | ||
| } else if (error instanceof Error) { | ||
| logger.error(error, `Failed to build ENSRainbowConfig`); | ||
| } else { | ||
| logger.error(`Unknown Error`); | ||
| } | ||
|
|
||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
process.exit(1) prevents testability and graceful error handling.
Calling process.exit(1) terminates the process immediately, making this function difficult to test and preventing callers from handling errors gracefully. Consider throwing a custom error and letting the caller decide how to handle it.
🔧 Suggested refactor for better error handling
+export class ConfigurationError extends Error {
+ constructor(message: string) {
+ super(message);
+ this.name = "ConfigurationError";
+ }
+}
+
export function buildConfigFromEnvironment(env: ENSRainbowEnvironment): ENSRainbowConfig {
try {
return ENSRainbowConfigSchema.parse({
// ... parsing logic
});
} catch (error) {
if (error instanceof ZodError) {
logger.error(`Failed to parse environment configuration: \n${prettifyError(error)}\n`);
+ throw new ConfigurationError(`Invalid configuration: ${prettifyError(error)}`);
} else if (error instanceof Error) {
logger.error(error, `Failed to build ENSRainbowConfig`);
+ throw error;
} else {
logger.error(`Unknown Error`);
+ throw new ConfigurationError("Unknown configuration error");
}
-
- process.exit(1);
}
}Then handle the exit at the call site (e.g., in CLI entry points):
try {
const config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment);
} catch (error) {
process.exit(1);
}🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 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.
Comment on lines
7
to
10
| export function getEnvPort(): number { | ||
| const envPort = process.env.PORT; | ||
| if (!envPort) { | ||
| return DEFAULT_PORT; | ||
| } | ||
|
|
||
| try { | ||
| const port = parseNonNegativeInteger(envPort); | ||
| return port; | ||
| } catch (_error: unknown) { | ||
| const errorMessage = `Invalid PORT value "${envPort}": must be a non-negative integer`; | ||
| logger.error(errorMessage); | ||
| throw new Error(errorMessage); | ||
| } | ||
| const config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment); | ||
| return config.port; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unsafe type assertion may mask configuration errors.
Casting process.env as ENSRainbowEnvironment bypasses TypeScript's type checking. While buildConfigFromEnvironment validates at runtime, the assertion could hide type mismatches during development.
Additionally, this function rebuilds the entire configuration on each call. If called frequently, consider memoizing or caching the result.
♻️ Suggested improvement
+let cachedConfig: ENSRainbowConfig | null = null;
+
/**
* Gets the port from environment variables.
*/
export function getEnvPort(): number {
- const config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment);
- return config.port;
+ if (!cachedConfig) {
+ cachedConfig = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment);
+ }
+ return cachedConfig.port;
}You'll also need to import the config type:
import type { ENSRainbowConfig } from "@/config/config.schema";🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/lib/env.ts` around lines 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).
Contributor
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.
Comment on lines
20
to
23
| if (config.dbSchemaVersion !== undefined && config.dbSchemaVersion !== DB_SCHEMA_VERSION) { | ||
| throw new Error( | ||
| `DB_SCHEMA_VERSION mismatch! Expected version ${DB_SCHEMA_VERSION} from code, but found ${config.dbSchemaVersion} in environment variables.`, | ||
| ); |
There was a problem hiding this comment.
This .check() invariant throws an Error directly. In most config invariants in this repo, the check adds a custom issue to ctx.issues so the failure is reported via ZodError and formatted by prettifyError (see apps/ensapi/src/config/validations.ts:18-24). Consider pushing a custom issue (with path: ["dbSchemaVersion"]) instead of throwing, so users get consistent, nicely formatted config errors.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensrainbow/src/lib/env.ts
Outdated
| @@ -1,24 +1,10 @@ | |||
| import { join } from "node:path"; | |||
| import { buildConfigFromEnvironment } from "@/config/config.schema"; | |||
There was a problem hiding this comment.
The refactored getEnvPort() function now calls process.exit(1) on validation failures instead of throwing an error, which breaks error handling in code paths that expect a catchable error. This causes the application to exit unexpectedly when validatePortConfiguration() is called with an invalid PORT environment variable.
View Details
📝 Patch Details
diff --git a/apps/ensrainbow/src/cli.test.ts b/apps/ensrainbow/src/cli.test.ts
index a8d674a4..b0159c00 100644
--- a/apps/ensrainbow/src/cli.test.ts
+++ b/apps/ensrainbow/src/cli.test.ts
@@ -51,20 +51,12 @@ describe("CLI", () => {
it("should throw error for invalid port number", () => {
process.env.PORT = "invalid";
- const exitSpy = vi.spyOn(process, "exit").mockImplementation((() => {
- throw new Error("process.exit called");
- }) as never);
expect(() => getEnvPort()).toThrow();
- expect(exitSpy).toHaveBeenCalledWith(1);
});
it("should throw error for negative port number", () => {
process.env.PORT = "-1";
- const exitSpy = vi.spyOn(process, "exit").mockImplementation((() => {
- throw new Error("process.exit called");
- }) as never);
expect(() => getEnvPort()).toThrow();
- expect(exitSpy).toHaveBeenCalledWith(1);
});
});
diff --git a/apps/ensrainbow/src/config/config.schema.ts b/apps/ensrainbow/src/config/config.schema.ts
index e74f166c..6e079dc5 100644
--- a/apps/ensrainbow/src/config/config.schema.ts
+++ b/apps/ensrainbow/src/config/config.schema.ts
@@ -71,14 +71,15 @@ export function buildConfigFromEnvironment(env: ENSRainbowEnvironment): ENSRainb
: undefined,
});
} catch (error) {
+ let errorMessage = `Failed to parse environment configuration`;
+
if (error instanceof ZodError) {
- logger.error(`Failed to parse environment configuration: \n${prettifyError(error)}\n`);
+ errorMessage = `Failed to parse environment configuration: \n${prettifyError(error)}\n`;
} else if (error instanceof Error) {
- logger.error(error, `Failed to build ENSRainbowConfig`);
- } else {
- logger.error(`Unknown Error`);
+ errorMessage = error.message;
}
- process.exit(1);
+ logger.error(errorMessage);
+ throw new Error(errorMessage);
}
}
Analysis
Invalid PORT environment variable prevents CLI port override
What fails: The refactored getEnvPort() function calls buildConfigFromEnvironment() which invokes process.exit(1) on validation failure. When an invalid PORT environment variable is set, the process exits immediately during CLI setup (via yargs default: option evaluation), preventing users from overriding it with --port CLI flag.
How to reproduce:
PORT=invalid node cli.ts serve --port 5000The process exits with validation error instead of using the provided --port 5000.
Expected vs Actual:
- Expected: Accept the
--port 5000CLI argument and use that port, either ignoring or properly validating the invalid PORT env var - Actual: Process exits immediately during yargs option setup before any command validation occurs
Root cause: In commit c34c0bd, getEnvPort() was refactored to call buildConfigFromEnvironment(), which catches validation errors and calls process.exit(1) instead of throwing a catchable error. The old implementation (commit b20b546) threw Error on validation failure, allowing calling code to catch and handle the error.
Fix applied: Changed buildConfigFromEnvironment() to throw Error instead of calling process.exit(1), allowing validation errors to propagate and be caught by calling code in validatePortConfiguration() and yargs error handlers.
… for environment variable handling
…d of environment variable
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/ensrainbow/src/config/config.schema.ts`:
- Around line 98-107: The parameter `config: ENSRainbowConfig` on the function
`buildENSRainbowPublicConfig` is unused; either remove it from the function
signature and update any callers that pass it, or rename it to `_config` to
signal intentional omission and satisfy linters; adjust the declaration of
`buildENSRainbowPublicConfig` accordingly and ensure imports/types referencing
`ENSRainbowConfig` remain consistent.
In `@apps/ensrainbow/src/lib/api.ts`:
- Around line 106-118: The /v1/config handler always returns HTTP 500 on
labelCount failures; change it to preserve the upstream error code from
server.labelCount(): when countResult.status === StatusCode.Error, call
c.json(...) with the HTTP status set to countResult.errorCode (falling back to
500 if errorCode is undefined) so the response aligns with /v1/labels/count;
refer to api.get("/v1/config", async (c: HonoContext)), server.labelCount(),
countResult.status, countResult.errorCode and c.json when making the change.
♻️ Duplicate comments (3)
apps/ensrainbow/src/config/config.schema.ts (3)
37-41: Avoid eagergetDefaultDataDir()evaluation at module load.
This capturesprocess.cwd()too early; prefer lazy evaluation at build time.♻️ Suggested refactor
const ENSRainbowConfigSchema = z .object({ port: PortSchema.default(ENSRAINBOW_DEFAULT_PORT), - dataDir: DataDirSchema.default(getDefaultDataDir()), + dataDir: DataDirSchema.optional(), dbSchemaVersion: DbSchemaVersionSchema, labelSet: LabelSetSchema.optional(), })return ENSRainbowConfigSchema.parse({ port: env.PORT, - dataDir: env.DATA_DIR, + dataDir: env.DATA_DIR ?? getDefaultDataDir(), dbSchemaVersion: env.DB_SCHEMA_VERSION, labelSet: env.LABEL_SET_ID || env.LABEL_SET_VERSION ? {
69-75: Treat empty label-set env vars as provided values.
Using a truthy check skips validation whenLABEL_SET_ID="". Switch to explicit!== undefinedchecks.🔧 Suggested fix
- labelSet: - env.LABEL_SET_ID || env.LABEL_SET_VERSION + labelSet: + env.LABEL_SET_ID !== undefined || env.LABEL_SET_VERSION !== undefined ? { labelSetId: env.LABEL_SET_ID, labelSetVersion: env.LABEL_SET_VERSION, } : undefined,
77-86: Avoidprocess.exit(1)in library code paths.
This makes callers/tests harder to handle; prefer throwing a typed error and let the entrypoint decide.🔧 Suggested refactor
- process.exit(1); + throw error instanceof Error ? error : new Error("Unknown configuration error");
Comment on lines
+98
to
+107
| export function buildENSRainbowPublicConfig( | ||
| config: ENSRainbowConfig, | ||
| labelSet: EnsRainbowServerLabelSet, | ||
| recordsCount: number, | ||
| ): EnsRainbow.ENSRainbowPublicConfig { | ||
| return { | ||
| version: packageJson.version, | ||
| labelSet, | ||
| recordsCount, | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Unused config parameter in buildENSRainbowPublicConfig.
If it’s intentionally unused, prefix with _ or remove to avoid confusion/lint noise.
♻️ Suggested tweak
-export function buildENSRainbowPublicConfig(
- config: ENSRainbowConfig,
+export function buildENSRainbowPublicConfig(
+ _config: ENSRainbowConfig,
labelSet: EnsRainbowServerLabelSet,
recordsCount: number,
): EnsRainbow.ENSRainbowPublicConfig {🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 98 - 107, The
parameter `config: ENSRainbowConfig` on the function
`buildENSRainbowPublicConfig` is unused; either remove it from the function
signature and update any callers that pass it, or rename it to `_config` to
signal intentional omission and satisfy linters; adjust the declaration of
`buildENSRainbowPublicConfig` accordingly and ensure imports/types referencing
`ENSRainbowConfig` remain consistent.
Comment on lines
+106
to
+118
| api.get("/v1/config", async (c: HonoContext) => { | ||
| logger.debug("Config request"); | ||
| const countResult = await server.labelCount(); | ||
| if (countResult.status === StatusCode.Error) { | ||
| logger.error("Failed to get records count for config endpoint"); | ||
| return c.json( | ||
| { | ||
| status: StatusCode.Error, | ||
| error: countResult.error, | ||
| errorCode: countResult.errorCode, | ||
| }, | ||
| 500, | ||
| ); |
There was a problem hiding this comment.
Preserve upstream error code for config failures.
Right now the handler always returns HTTP 500, even if countResult.errorCode is more specific. Align with /v1/labels/count by returning the upstream code.
🔧 Suggested fix
- 500,
+ countResult.errorCode ?? 500,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| api.get("/v1/config", async (c: HonoContext) => { | |
| logger.debug("Config request"); | |
| const countResult = await server.labelCount(); | |
| if (countResult.status === StatusCode.Error) { | |
| logger.error("Failed to get records count for config endpoint"); | |
| return c.json( | |
| { | |
| status: StatusCode.Error, | |
| error: countResult.error, | |
| errorCode: countResult.errorCode, | |
| }, | |
| 500, | |
| ); | |
| api.get("/v1/config", async (c: HonoContext) => { | |
| logger.debug("Config request"); | |
| const countResult = await server.labelCount(); | |
| if (countResult.status === StatusCode.Error) { | |
| logger.error("Failed to get records count for config endpoint"); | |
| return c.json( | |
| { | |
| status: StatusCode.Error, | |
| error: countResult.error, | |
| errorCode: countResult.errorCode, | |
| }, | |
| countResult.errorCode ?? 500, | |
| ); |
🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/lib/api.ts` around lines 106 - 118, The /v1/config
handler always returns HTTP 500 on labelCount failures; change it to preserve
the upstream error code from server.labelCount(): when countResult.status ===
StatusCode.Error, call c.json(...) with the HTTP status set to
countResult.errorCode (falling back to 500 if errorCode is undefined) so the
response aligns with /v1/labels/count; refer to api.get("/v1/config", async (c:
HonoContext)), server.labelCount(), countResult.status, countResult.errorCode
and c.json when making the change.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 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.
Comment on lines
+98
to
+108
| export function buildENSRainbowPublicConfig( | ||
| config: ENSRainbowConfig, | ||
| labelSet: EnsRainbowServerLabelSet, | ||
| recordsCount: number, | ||
| ): EnsRainbow.ENSRainbowPublicConfig { | ||
| return { | ||
| version: packageJson.version, | ||
| labelSet, | ||
| recordsCount, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The config parameter in buildENSRainbowPublicConfig is declared but never used in the function body. The function only uses packageJson.version, labelSet, and recordsCount parameters. Either remove the unused config parameter or use it if there's a future need for it. Based on the similar pattern in ENSApi (buildEnsApiPublicConfig), the config parameter is used to access configuration values. Consider whether any config values should be included in the public config.
Comment on lines
+98
to
+108
| export function buildENSRainbowPublicConfig( | ||
| config: ENSRainbowConfig, | ||
| labelSet: EnsRainbowServerLabelSet, | ||
| recordsCount: number, | ||
| ): EnsRainbow.ENSRainbowPublicConfig { | ||
| return { | ||
| version: packageJson.version, | ||
| labelSet, | ||
| recordsCount, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The buildENSRainbowPublicConfig function is not tested. Consider adding tests to verify that it correctly builds the public config from the provided parameters (config, labelSet, recordsCount). This would ensure the function works as expected and maintains consistency with similar functions in ENSApi and ENSIndexer.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/ensrainbow/src/config/config.schema.test.ts`:
- Around line 3-28: The test suite currently replaces process.exit with a spy
(mockExit) but never restores it, which can leak into other tests; add an
afterAll hook that calls mockExit.mockRestore() to restore the original
process.exit. Locate the mock created with vi.spyOn(process, "exit") assigned to
mockExit in this file and add an afterAll(() => mockExit.mockRestore()) to
ensure the spy is removed after the suite finishes.
Comment on lines
3
to
28
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | ||
|
|
||
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | ||
| import { logger } from "@/utils/logger"; | ||
|
|
||
| import { buildConfigFromEnvironment } from "./config.schema"; | ||
| import { ENSRAINBOW_DEFAULT_PORT, getDefaultDataDir } from "./defaults"; | ||
| import type { ENSRainbowEnvironment } from "./environment"; | ||
|
|
||
| vi.mock("@/utils/logger", () => ({ | ||
| logger: { | ||
| error: vi.fn(), | ||
| }, | ||
| })); | ||
|
|
||
| describe("buildConfigFromEnvironment", () => { | ||
| // Mock process.exit to prevent actual exit | ||
| const mockExit = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| mockExit.mockClear(); | ||
| }); |
There was a problem hiding this comment.
Restore the process.exit spy after the suite completes.
The spy is never restored, so other test files in the same worker can inherit the mocked process.exit and mask real exits. Add an afterAll restore.
🧹 Proposed fix
-import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from "vitest";
@@
afterEach(() => {
mockExit.mockClear();
});
+
+ afterAll(() => {
+ mockExit.mockRestore();
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | |
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | |
| import { logger } from "@/utils/logger"; | |
| import { buildConfigFromEnvironment } from "./config.schema"; | |
| import { ENSRAINBOW_DEFAULT_PORT, getDefaultDataDir } from "./defaults"; | |
| import type { ENSRainbowEnvironment } from "./environment"; | |
| vi.mock("@/utils/logger", () => ({ | |
| logger: { | |
| error: vi.fn(), | |
| }, | |
| })); | |
| describe("buildConfigFromEnvironment", () => { | |
| // Mock process.exit to prevent actual exit | |
| const mockExit = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| afterEach(() => { | |
| mockExit.mockClear(); | |
| }); | |
| import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from "vitest"; | |
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | |
| import { logger } from "@/utils/logger"; | |
| import { buildConfigFromEnvironment } from "./config.schema"; | |
| import { ENSRAINBOW_DEFAULT_PORT, getDefaultDataDir } from "./defaults"; | |
| import type { ENSRainbowEnvironment } from "./environment"; | |
| vi.mock("@/utils/logger", () => ({ | |
| logger: { | |
| error: vi.fn(), | |
| }, | |
| })); | |
| describe("buildConfigFromEnvironment", () => { | |
| // Mock process.exit to prevent actual exit | |
| const mockExit = vi.spyOn(process, "exit").mockImplementation(() => undefined as never); | |
| beforeEach(() => { | |
| vi.clearAllMocks(); | |
| }); | |
| afterEach(() => { | |
| mockExit.mockClear(); | |
| }); | |
| afterAll(() => { | |
| mockExit.mockRestore(); | |
| }); |
🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.test.ts` around lines 3 - 28, The
test suite currently replaces process.exit with a spy (mockExit) but never
restores it, which can leak into other tests; add an afterAll hook that calls
mockExit.mockRestore() to restore the original process.exit. Locate the mock
created with vi.spyOn(process, "exit") assigned to mockExit in this file and add
an afterAll(() => mockExit.mockRestore()) to ensure the spy is removed after the
suite finishes.
…ror handling in buildConfigFromEnvironment
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Comment on lines
+97
to
+98
| labelSetId: env.LABEL_SET_ID, | ||
| labelSetVersion: env.LABEL_SET_VERSION, |
There was a problem hiding this comment.
Suggested change
| labelSetId: env.LABEL_SET_ID, | |
| labelSetVersion: env.LABEL_SET_VERSION, | |
| labelSetId: env.LABEL_SET_ID!.trim(), | |
| labelSetVersion: env.LABEL_SET_VERSION!.trim(), |
The LABEL_SET_ID and LABEL_SET_VERSION values are not trimmed before being returned from the transform, even though the code checks that the trimmed values are non-empty. This will cause validation to fail if either value has leading or trailing whitespace.
View Details
Analysis
Untrimmed LABEL_SET_ID and LABEL_SET_VERSION values fail regex validation
What fails: The buildConfigFromEnvironment() function validates that trimmed LABEL_SET_ID and LABEL_SET_VERSION are non-empty, but then passes the untrimmed values to LabelSetSchema. Since makeLabelSetIdSchema validates against /^[a-z-]+$/, strings with leading/trailing whitespace fail validation despite passing the trim check.
How to reproduce:
const env = {
LABEL_SET_ID: " subgraph ",
LABEL_SET_VERSION: "0"
};
buildConfigFromEnvironment(env); // Throws ZodError with "LABEL_SET_ID can only contain lowercase letters (a-z) and hyphens (-)"Result: Validation fails with regex format error: code: "invalid_format", pattern: "/^[a-z-]+$/"
Expected: Should accept the environment variables and return config with trimmed values: {labelSetId: "subgraph", labelSetVersion: 0}
Fix: Added .trim() calls when constructing the labelSet object to match the trimmed values already validated earlier in the transform function (lines 97-98 in apps/ensrainbow/src/config/config.schema.ts). This aligns with how DATA_DIR is handled in the schema.
Contributor
Greptile OverviewGreptile SummaryThis PR implements a comprehensive configuration system for ENSRainbow using Zod validation, addressing issue #1407. Key Changes:
Architecture:
Testing:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant ENV as Environment Variables
participant CI as config/index.ts
participant CS as config/config.schema.ts
participant ZOD as Zod Validation
participant CLI as cli.ts
participant API as lib/api.ts
participant Client as ensrainbow-sdk
Note over ENV,CI: Module Load Time (Startup)
CI->>CS: buildConfigFromEnvironment(process.env)
CS->>ZOD: Parse PORT, DATA_DIR, DB_SCHEMA_VERSION, LABEL_SET_*
ZOD->>ZOD: Validate schema & invariants
alt Validation Success
ZOD-->>CS: Validated ENSRainbowConfig
CS-->>CI: config object
CI->>CI: export default config
else Validation Error
ZOD-->>CS: ZodError / Error
CS-->>CI: throw error
CI->>CI: console.error + process.exit(1)
end
Note over CLI,API: Runtime (Server/CLI Operations)
CLI->>CI: import config
CLI->>CLI: Use config.port for defaults
CLI->>API: serverCommand(port, dataDir)
Note over API,Client: New Config Endpoint
Client->>API: GET /v1/config
API->>API: server.labelCount()
API->>CS: buildENSRainbowPublicConfig(config, labelSet, count)
CS-->>API: ENSRainbowPublicConfig
API-->>Client: { version, labelSet, recordsCount }
Note over API,Client: Deprecated Version Endpoint
Client->>API: GET /v1/version (deprecated)
API-->>Client: { status, versionInfo }
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| config = buildConfigFromEnvironment(process.env as ENSRainbowEnvironment); | ||
| } catch (error) { | ||
| // For CLI applications, invalid configuration should exit the process | ||
| console.error("Configuration error:", error instanceof Error ? error.message : String(error)); |
Contributor
There was a problem hiding this comment.
console.error doesn't output structured error details from Zod validation. Consider using logger.error and formatting ZodError for better debugging
Suggested change
| console.error("Configuration error:", error instanceof Error ? error.message : String(error)); | |
| logger.error("Configuration error:", error instanceof ZodError ? error.format() : error); |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ensrainbow/src/config/index.ts
Line: 15:15
Comment:
`console.error` doesn't output structured error details from Zod validation. Consider using `logger.error` and formatting ZodError for better debugging
```suggestion
logger.error("Configuration error:", error instanceof ZodError ? error.format() : error);
```
<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>
How can I resolve this? If you propose a fix, please make it concise.
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.
Related to #1407