Add env helpers and refactor config #401
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #401 +/- ##
===========================================
+ Coverage 72.91% 73.01% +0.10%
===========================================
Files 49 49
Lines 12348 12349 +1
Branches 706 702 -4
===========================================
+ Hits 9003 9017 +14
+ Misses 3339 3326 -13
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe configuration system was refactored to use new typed environment variable accessors from the Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant EnvHelpers
App->>EnvHelpers: intVar('PORT', 4000)
EnvHelpers-->>App: 4000 (number)
App->>EnvHelpers: boolVar('REDIS_USE_TLS', false)
EnvHelpers-->>App: false (boolean)
App->>EnvHelpers: listVar('CHUNK_POST_URLS', default)
EnvHelpers-->>App: [url1, url2, ...]
App->>EnvHelpers: jsonVar('ANS104_UNBUNDLE_FILTER_PARSED', {})
EnvHelpers-->>App: parsedObject
App->>EnvHelpers: urlVar('AO_MU_URL')
EnvHelpers-->>App: sanitizedUrl or undefined
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
yarn install v1.22.22 ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/config.ts (2)
106-119: 🛠️ Refactor suggestionInconsistent refactoring: Some manual parsing remains.
Several environment variables still use the old
+env.varOrDefault()pattern instead of the newintVarutility.For consistency, convert these to use
intVar:-export const TRUSTED_GATEWAYS_REQUEST_TIMEOUT_MS = +env.varOrDefault( - 'TRUSTED_GATEWAYS_REQUEST_TIMEOUT_MS', - '10000', -); +export const TRUSTED_GATEWAYS_REQUEST_TIMEOUT_MS = env.intVar( + 'TRUSTED_GATEWAYS_REQUEST_TIMEOUT_MS', + 10000, +); -export const WEIGHTED_PEERS_TEMPERATURE_DELTA = +env.varOrDefault( - 'WEIGHTED_PEERS_TEMPERATURE_DELTA', - '2', -); +export const WEIGHTED_PEERS_TEMPERATURE_DELTA = env.intVar( + 'WEIGHTED_PEERS_TEMPERATURE_DELTA', + 2, +); -export const GATEWAY_PEERS_WEIGHTS_CACHE_DURATION_MS = +env.varOrDefault( - 'GATEWAY_PEERS_WEIGHTS_CACHE_DURATION_MS', - `${5 * 1000}`, // 5 seconds -); +export const GATEWAY_PEERS_WEIGHTS_CACHE_DURATION_MS = env.intVar( + 'GATEWAY_PEERS_WEIGHTS_CACHE_DURATION_MS', + 5 * 1000, // 5 seconds +);
197-200: 🛠️ Refactor suggestionAdditional manual parsing instances to refactor.
More instances of the old parsing pattern that should be converted for consistency.
Apply similar refactoring here:
-export const ARWEAVE_PEER_CHUNK_POST_CONCURRENCY_LIMIT = +env.varOrDefault( - 'ARWEAVE_PEER_CHUNK_POST_CONCURRENCY_LIMIT', - '3', -); +export const ARWEAVE_PEER_CHUNK_POST_CONCURRENCY_LIMIT = env.intVar( + 'ARWEAVE_PEER_CHUNK_POST_CONCURRENCY_LIMIT', + 3, +);
🧹 Nitpick comments (2)
src/lib/env.ts (2)
42-44: Consider case-insensitive boolean parsing for better usability.The current implementation only accepts lowercase 'true' as truthy, which might be surprising to users who set environment variables like "TRUE" or "True".
Consider this more flexible approach:
-export function boolVar(envVarName: string, defaultValue: boolean): boolean { - return varOrDefault(envVarName, defaultValue ? 'true' : 'false') === 'true'; -} +export function boolVar(envVarName: string, defaultValue: boolean): boolean { + return varOrDefault(envVarName, defaultValue ? 'true' : 'false').toLowerCase() === 'true'; +}
55-62: Review list filtering behavior for edge cases.The
filter(Boolean)approach removes falsy values including "0", "false", and empty strings. This might unexpectedly filter out valid list items.Consider filtering only empty strings if "0" or "false" are valid list items:
export function listVar(envVarName: string, defaultValue: string): string[] { - return varOrDefault(envVarName, defaultValue).split(',').filter(Boolean); + return varOrDefault(envVarName, defaultValue).split(',').filter(item => item.trim() !== ''); } export function optionalListVar(envVarName: string): string[] { const value = varOrUndefined(envVarName); - return value !== undefined ? value.split(',').filter(Boolean) : []; + return value !== undefined ? value.split(',').filter(item => item.trim() !== '') : []; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/config.ts(16 hunks)src/lib/env.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: test (macos-latest)
🔇 Additional comments (6)
src/lib/env.ts (1)
68-71: URL sanitization implementation looks good.The function correctly handles optional URLs and removes trailing slashes using a robust regex pattern.
src/config.ts (5)
32-32: Port configuration conversion looks correct.The conversion from manual parsing to
intVarpreserves the default value and improves type safety.
61-61: Boolean environment variable conversions are correct.The refactoring properly converts manual string comparisons to typed
boolVarcalls while preserving default values.Also applies to: 248-248, 317-321
129-131: List parsing conversions look good.The conversion from manual string splitting to
listVarandoptionalListVarmaintains the same behavior while improving readability.Also applies to: 134-137, 144-146
802-805: URL variable refactoring is well implemented.The conversion to
urlVarproperly handles URL sanitization and maintains the fallback logic with nullish coalescing operators.
255-258: JSON parsing conversions preserve functionality.The refactoring from manual
JSON.parsetojsonVarmaintains the same default values and behavior.Also applies to: 268-270
| export function jsonVar<T>(envVarName: string, defaultValue: T): T { | ||
| return JSON.parse(varOrDefault(envVarName, JSON.stringify(defaultValue))); | ||
| } |
There was a problem hiding this comment.
Add error handling for JSON parsing.
The jsonVar function uses JSON.parse without error handling, which will throw on malformed JSON and crash the application during startup.
Add error handling:
export function jsonVar<T>(envVarName: string, defaultValue: T): T {
- return JSON.parse(varOrDefault(envVarName, JSON.stringify(defaultValue)));
+ try {
+ return JSON.parse(varOrDefault(envVarName, JSON.stringify(defaultValue)));
+ } catch (error) {
+ throw new Error(`Invalid JSON value for ${envVarName}: ${error.message}`);
+ }
}📝 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.
| export function jsonVar<T>(envVarName: string, defaultValue: T): T { | |
| return JSON.parse(varOrDefault(envVarName, JSON.stringify(defaultValue))); | |
| } | |
| export function jsonVar<T>(envVarName: string, defaultValue: T): T { | |
| try { | |
| return JSON.parse(varOrDefault(envVarName, JSON.stringify(defaultValue))); | |
| } catch (error) { | |
| throw new Error(`Invalid JSON value for ${envVarName}: ${error.message}`); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/lib/env.ts around lines 64 to 66, the jsonVar function calls JSON.parse
without error handling, which can cause the application to crash if the JSON is
malformed. Wrap the JSON.parse call in a try-catch block to catch any parsing
errors. If an error occurs, log or handle it appropriately and return the
defaultValue instead of throwing, ensuring the application startup is not
interrupted by malformed JSON.
| export function intVar(envVarName: string, defaultValue: number): number { | ||
| return parseInt(varOrDefault(envVarName, String(defaultValue)), 10); | ||
| } | ||
|
|
||
| export function optionalIntVar(envVarName: string): number | undefined { | ||
| const value = varOrUndefined(envVarName); | ||
| return value !== undefined ? parseInt(value, 10) : undefined; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid integer values.
Both intVar and optionalIntVar use parseInt without error handling. Invalid values will return NaN, which could cause unexpected behavior in the application.
Consider adding validation:
export function intVar(envVarName: string, defaultValue: number): number {
- return parseInt(varOrDefault(envVarName, String(defaultValue)), 10);
+ const parsed = parseInt(varOrDefault(envVarName, String(defaultValue)), 10);
+ if (isNaN(parsed)) {
+ throw new Error(`Invalid integer value for ${envVarName}`);
+ }
+ return parsed;
}
export function optionalIntVar(envVarName: string): number | undefined {
const value = varOrUndefined(envVarName);
- return value !== undefined ? parseInt(value, 10) : undefined;
+ if (value === undefined) return undefined;
+ const parsed = parseInt(value, 10);
+ if (isNaN(parsed)) {
+ throw new Error(`Invalid integer value for ${envVarName}`);
+ }
+ return parsed;
}📝 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.
| export function intVar(envVarName: string, defaultValue: number): number { | |
| return parseInt(varOrDefault(envVarName, String(defaultValue)), 10); | |
| } | |
| export function optionalIntVar(envVarName: string): number | undefined { | |
| const value = varOrUndefined(envVarName); | |
| return value !== undefined ? parseInt(value, 10) : undefined; | |
| } | |
| export function intVar(envVarName: string, defaultValue: number): number { | |
| const parsed = parseInt(varOrDefault(envVarName, String(defaultValue)), 10); | |
| if (isNaN(parsed)) { | |
| throw new Error(`Invalid integer value for ${envVarName}`); | |
| } | |
| return parsed; | |
| } | |
| export function optionalIntVar(envVarName: string): number | undefined { | |
| const value = varOrUndefined(envVarName); | |
| if (value === undefined) return undefined; | |
| const parsed = parseInt(value, 10); | |
| if (isNaN(parsed)) { | |
| throw new Error(`Invalid integer value for ${envVarName}`); | |
| } | |
| return parsed; | |
| } |
🤖 Prompt for AI Agents
In src/lib/env.ts around lines 46 to 53, the functions intVar and optionalIntVar
use parseInt without validating the result, which can lead to NaN values if the
environment variable is not a valid integer. To fix this, add checks after
parseInt to verify the parsed value is a valid number (not NaN). For intVar,
throw an error or handle the invalid case if parseInt returns NaN. For
optionalIntVar, return undefined or handle the invalid case similarly when
parseInt yields NaN.
| export const STOP_HEIGHT = +env.varOrDefault('STOP_HEIGHT', 'Infinity'); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mixed parsing patterns for numeric values.
Line 252 uses the new intVar while line 253 still uses the old +env.varOrDefault() pattern.
Convert for consistency:
export const START_HEIGHT = env.intVar('START_HEIGHT', 0);
-export const STOP_HEIGHT = +env.varOrDefault('STOP_HEIGHT', 'Infinity');
+export const STOP_HEIGHT = env.intVar('STOP_HEIGHT', Infinity);📝 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.
| export const STOP_HEIGHT = +env.varOrDefault('STOP_HEIGHT', 'Infinity'); | |
| export const START_HEIGHT = env.intVar('START_HEIGHT', 0); | |
| export const STOP_HEIGHT = env.intVar('STOP_HEIGHT', Infinity); |
🤖 Prompt for AI Agents
In src/config.ts around lines 252 to 253, the code uses mixed patterns for
parsing numeric values: line 252 uses the new intVar method while line 253 uses
the old +env.varOrDefault() pattern. To fix this, replace the usage of
+env.varOrDefault('STOP_HEIGHT', 'Infinity') with the intVar method to maintain
consistency in numeric parsing across the file.
Summary
config.tsto use new helpersenv.urlVarTesting
npx eslint src testyarn test(fails: This package doesn't seem to be present in your lockfile)