Build ENSRainbow config #1425
Build ENSRainbow config #1425
Conversation
…NSRainbow application
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
|
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, adds environment typing and validation, refactors CLI and env handling to consume the new config, updates tests to use new defaults, and tightens port validation in the shared SDK. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "CLI"
participant Config as "config (buildConfigFromEnvironment)"
participant Zod as "Zod validation"
participant Process as "process"
CLI->>Config: request config (use process.env)
Config->>Zod: parse/validate environment
Zod-->>Config: validation result (ok / errors)
alt validation OK
Config-->>CLI: return ENSRainbowConfig (port, dataDir, ...)
CLI->>CLI: continue startup (serve/ingest/etc.)
else validation FAIL
Zod-->>Process: throw/prettified errors
Process->>Process: process.exit(1)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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).
| .transform((path: string) => { | ||
| // Resolve relative paths to absolute paths | ||
| if (path.startsWith("/")) { | ||
| return path; | ||
| } | ||
| return join(process.cwd(), path); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Path resolution doesn't handle Windows absolute paths.
The transform only checks for / prefix to identify absolute paths, which won't work correctly on Windows (e.g., C:\data). Consider using path.isAbsolute() for cross-platform support.
♻️ Cross-platform fix
+import { isAbsolute, join } from "node:path";
-import { join } from "node:path";
const DataDirSchema = z
.string()
.trim()
.min(1, {
error: "DATA_DIR must be a non-empty string.",
})
.transform((path: string) => {
// Resolve relative paths to absolute paths
- if (path.startsWith("/")) {
+ if (isAbsolute(path)) {
return path;
}
return join(process.cwd(), path);
});🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 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.
| 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.
| labelSet: | ||
| env.LABEL_SET_ID || env.LABEL_SET_VERSION | ||
| ? { | ||
| labelSetId: env.LABEL_SET_ID, | ||
| labelSetVersion: env.LABEL_SET_VERSION, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Using || treats empty strings as missing values.
The condition env.LABEL_SET_ID || env.LABEL_SET_VERSION will treat empty strings "" as falsy. If either variable is explicitly set to an empty string, the label set will be undefined rather than triggering a validation error. Use !== undefined for explicit presence checks.
♻️ Explicit undefined check
labelSet:
- env.LABEL_SET_ID || env.LABEL_SET_VERSION
+ env.LABEL_SET_ID !== undefined || env.LABEL_SET_VERSION !== undefined
? {
labelSetId: env.LABEL_SET_ID,
labelSetVersion: env.LABEL_SET_VERSION,
}
: undefined,📝 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.
| labelSet: | |
| env.LABEL_SET_ID || env.LABEL_SET_VERSION | |
| ? { | |
| labelSetId: env.LABEL_SET_ID, | |
| labelSetVersion: env.LABEL_SET_VERSION, | |
| } | |
| : undefined, | |
| labelSet: | |
| env.LABEL_SET_ID !== undefined || env.LABEL_SET_VERSION !== undefined | |
| ? { | |
| labelSetId: env.LABEL_SET_ID, | |
| labelSetVersion: env.LABEL_SET_VERSION, | |
| } | |
| : undefined, |
🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/config.schema.ts` around lines 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.
| } 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.
| /** | ||
| * Zod `.check()` function input. | ||
| */ | ||
| type ZodCheckFnInput<T> = z.core.ParsePayload<T>; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using standard Zod types instead of internal z.core namespace.
z.core.ParsePayload<T> is from Zod's internal API surface and may be unstable across versions. Consider defining a simpler interface or using documented Zod 4 check function patterns.
♻️ Alternative approach using a simpler type
-/**
- * Zod `.check()` function input.
- */
-type ZodCheckFnInput<T> = z.core.ParsePayload<T>;
+/**
+ * Zod `.check()` function input context.
+ */
+interface ZodCheckFnInput<T> {
+ value: T;
+ issues: { push: (issue: unknown) => void };
+}📝 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.
| /** | |
| * Zod `.check()` function input. | |
| */ | |
| type ZodCheckFnInput<T> = z.core.ParsePayload<T>; | |
| /** | |
| * Zod `.check()` function input context. | |
| */ | |
| interface ZodCheckFnInput<T> { | |
| value: T; | |
| issues: { push: (issue: unknown) => void }; | |
| } |
🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/config/validations.ts` around lines 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.
| 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).
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.
| export function buildConfigFromEnvironment(env: ENSRainbowEnvironment): ENSRainbowConfig { | ||
| try { | ||
| return ENSRainbowConfigSchema.parse({ | ||
| port: env.PORT, | ||
| dataDir: env.DATA_DIR, |
There was a problem hiding this comment.
buildConfigFromEnvironment introduces new parsing/validation behavior (DATA_DIR normalization, DB_SCHEMA_VERSION invariant, LABEL_SET pinning, defaulting). Other apps in this repo have dedicated config schema tests (e.g. apps/ensapi/src/config/config.schema.test.ts), but ENSRainbow doesn’t yet. Adding a small config.schema.test.ts for success and failure cases would help prevent regressions.
| .min(1, { error: "PORT must be greater than 1." }) | ||
| .max(65535, { error: "PORT must be less than 65535" }) |
There was a problem hiding this comment.
PortSchema's range error messages don't match the actual validation: .min(1) allows 1 but the message says "greater than 1", and .max(65535) allows 65535 but the message says "less than 65535" (also missing trailing period). Please align the messages with the inclusive bounds (or adjust the bounds to match the wording).
| .min(1, { error: "PORT must be greater than 1." }) | |
| .max(65535, { error: "PORT must be less than 65535" }) | |
| .min(1, { error: "PORT must be greater than or equal to 1." }) | |
| .max(65535, { error: "PORT must be less than or equal to 65535." }) |
| // Resolve relative paths to absolute paths | ||
| if (path.startsWith("/")) { | ||
| return path; | ||
| } | ||
| return join(process.cwd(), path); |
There was a problem hiding this comment.
DATA_DIR path normalization treats a path as absolute only if it starts with "/". This breaks on Windows (e.g., C:\... or UNC paths) and can also break the default data dir on Windows. Prefer path.isAbsolute() / path.resolve() from node:path to detect/resolve absolute vs relative paths in a cross-platform way.
| env.LABEL_SET_ID || env.LABEL_SET_VERSION | ||
| ? { | ||
| labelSetId: env.LABEL_SET_ID, | ||
| labelSetVersion: env.LABEL_SET_VERSION, | ||
| } |
There was a problem hiding this comment.
The labelSet construction uses truthiness (env.LABEL_SET_ID || env.LABEL_SET_VERSION) to decide whether to parse a label set. If either env var is set to an empty string, this evaluates falsy and silently skips validation. Consider checking for !== undefined (or trimming) so empty values fail validation instead of being ignored.
| import type { z } from "zod/v4"; | ||
|
|
||
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | ||
|
|
||
| import type { ENSRainbowConfig } from "./config.schema"; | ||
|
|
||
| /** | ||
| * Zod `.check()` function input. | ||
| */ | ||
| type ZodCheckFnInput<T> = z.core.ParsePayload<T>; | ||
|
|
||
| /** |
There was a problem hiding this comment.
ZodCheckFnInput is duplicated here, but the repo already exports this type from @ensnode/ensnode-sdk/internal (e.g. apps/ensapi/src/config/validations.ts:4 or packages/ensnode-sdk/src/shared/zod-types.ts:6). Importing the shared type would avoid drift if Zod internals change.
| import type { z } from "zod/v4"; | |
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | |
| import type { ENSRainbowConfig } from "./config.schema"; | |
| /** | |
| * Zod `.check()` function input. | |
| */ | |
| type ZodCheckFnInput<T> = z.core.ParsePayload<T>; | |
| /** | |
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | |
| import type { ENSRainbowConfig } from "./config.schema"; | |
| import type { ZodCheckFnInput } from "@ensnode/ensnode-sdk/internal"; | |
| /** |
| 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.
Pull request overview
Copilot reviewed 11 out of 12 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.
| import type { z } from "zod/v4"; | ||
|
|
||
| import { DB_SCHEMA_VERSION } from "@/lib/database"; | ||
|
|
||
| import type { ENSRainbowConfig } from "./config.schema"; | ||
|
|
||
| /** | ||
| * Zod `.check()` function input. | ||
| */ | ||
| type ZodCheckFnInput<T> = z.core.ParsePayload<T>; |
There was a problem hiding this comment.
This file redefines ZodCheckFnInput via z.core.ParsePayload, but the repo already has a shared ZodCheckFnInput type exported from @ensnode/ensnode-sdk/internal (see apps/ensapi/src/config/validations.ts:4). Reusing the shared type avoids duplicating Zod-internal typings across apps.
| it("should return ENSRAINBOW_DEFAULT_PORT when PORT is not set", () => { | ||
| expect(getEnvPort()).toBe(ENSRAINBOW_DEFAULT_PORT); |
There was a problem hiding this comment.
getEnvPort() reads from the module-level config built at import time. This test calls the already-imported getEnvPort() without resetting modules, so it can become environment-dependent (e.g. if PORT is set when Vitest starts). To make it deterministic, reset modules + re-import after stubbing PORT (similar to the other tests in this suite).
| it("should return ENSRAINBOW_DEFAULT_PORT when PORT is not set", () => { | |
| expect(getEnvPort()).toBe(ENSRAINBOW_DEFAULT_PORT); | |
| it("should return ENSRAINBOW_DEFAULT_PORT when PORT is not set", async () => { | |
| vi.resetModules(); | |
| const { getEnvPort: getEnvPortFresh } = await import("@/lib/env"); | |
| expect(getEnvPortFresh()).toBe(ENSRAINBOW_DEFAULT_PORT); |
| it("should return ENSRAINBOW_DEFAULT_PORT when PORT is not set", () => { | ||
| expect(getEnvPort()).toBe(ENSRAINBOW_DEFAULT_PORT); |
There was a problem hiding this comment.
| it("should return ENSRAINBOW_DEFAULT_PORT when PORT is not set", () => { | |
| expect(getEnvPort()).toBe(ENSRAINBOW_DEFAULT_PORT); | |
| it("should return ENSRAINBOW_DEFAULT_PORT when PORT is not set", async () => { | |
| vi.resetModules(); | |
| const { getEnvPort: getEnvPortFresh } = await import("@/lib/env"); | |
| expect(getEnvPortFresh()).toBe(ENSRAINBOW_DEFAULT_PORT); |
The test "should return ENSRAINBOW_DEFAULT_PORT when PORT is not set" will fail if the PORT environment variable is set when the test file is imported, because the config is now evaluated at module load time and the test doesn't reload the modules to reset it.
View Details
Analysis
Test fails when PORT environment variable is set at test startup
What fails: The test "should return ENSRAINBOW_DEFAULT_PORT when PORT is not set" in apps/ensrainbow/src/cli.test.ts (line 42-44) fails if the PORT environment variable is set when tests start, because the config module evaluates at import time, not test execution time.
How to reproduce:
cd apps/ensrainbow
PORT=5000 pnpm test cli.test.tsResult: Test fails with:
AssertionError: expected 5000 to be 3223 // Object.is equality
Expected: Test should pass and getEnvPort() should return the default port 3223.
Root cause: The config is built at module import time (in src/config/index.ts) via buildConfigFromEnvironment(process.env). When PORT is set in the environment before tests run, the config module caches this value. Unlike the other tests in the getEnvPort describe block, the first test doesn't call vi.resetModules() to reload the config with fresh environment variables. Since vi.stubEnv("PORT", undefined) in the beforeEach hook runs after module import, it has no effect on the cached config.
Fix: Make the first test consistent with the other tests in the describe block by calling vi.resetModules() after the beforeEach hook sets PORT to undefined, then re-import getEnvPort to get a fresh config instance.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensrainbow/src/cli.ts (1)
134-138: Consider usingconfig.portdirectly instead ofgetEnvPort().Since
getEnvPort()simply returnsconfig.port, andconfigis already imported, you could simplify:.option("port", { type: "number", description: "Port to listen on", - default: getEnvPort(), + default: config.port, })This removes the indirection and makes it clear the default comes from config. This would also allow removing the
getEnvPortimport entirely if not used elsewhere.
🤖 Fix all issues with AI agents
In `@apps/ensrainbow/src/cli.ts`:
- Around line 1-2: Import paths are inconsistent: you're importing config from
"@/config" but getDefaultDataDir from "@/config/defaults"; switch to importing
getDefaultDataDir from the barrel export so both come from "@/config" (replace
the direct "@/config/defaults" import with a named import from "@/config"),
update any occurrences referencing getDefaultDataDir in the file (including the
occurrences around lines 19-20) to use the consolidated import, and remove the
unused direct defaults import.
| import config from "@/config"; | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor inconsistency in import paths.
The file imports config from @/config but getDefaultDataDir directly from @/config/defaults. Since getDefaultDataDir is re-exported from @/config/index.ts, consider consolidating:
import config from "@/config";
+import { getDefaultDataDir } from "@/config";
import { join, resolve } from "node:path";
...
-import { getDefaultDataDir } from "@/config/defaults";
import { getEnvPort } from "@/lib/env";This keeps import paths consistent and relies on the barrel export pattern.
Also applies to: 19-20
🤖 Prompt for AI Agents
In `@apps/ensrainbow/src/cli.ts` around lines 1 - 2, Import paths are
inconsistent: you're importing config from "@/config" but getDefaultDataDir from
"@/config/defaults"; switch to importing getDefaultDataDir from the barrel
export so both come from "@/config" (replace the direct "@/config/defaults"
import with a named import from "@/config"), update any occurrences referencing
getDefaultDataDir in the file (including the occurrences around lines 19-20) to
use the consolidated import, and remove the unused direct defaults import.
| */ | ||
| .check(invariant_dbSchemaVersionMatch); | ||
|
|
||
| export type ENSRainbowConfig = z.infer<typeof ENSRainbowConfigSchema>; |
There was a problem hiding this comment.
To stay aligned with rest of the codebase, I suggest replacing ENS* type name prefix with Ens*.
| export type ENSRainbowConfig = z.infer<typeof ENSRainbowConfigSchema>; | |
| export type EnsRainbowConfig = z.infer<typeof EnsRainbowConfigSchema>; |
Related to #1407