chore: remove localhost value for AGENT_INTERNAL_URL #224
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Environment
participant API
Client->>Environment: Check AGENT_INTERNAL_URL
alt AGENT_INTERNAL_URL is set
Environment-->>Client: Return AGENT_INTERNAL_URL
Client->>API: Fetch data using FLATFILE_API_URL
else AGENT_INTERNAL_URL is not set
Environment-->>Client: Throw error "AGENT_INTERNAL_URL must be set in the environment"
end
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
packages/listener/src/events/event.handler.spec.tsOops! Something went wrong! :( ESLint: 8.57.1 Error [ERR_REQUIRE_ESM]: require() of ES Module /packages/listener/.eslintrc.js from /node_modules/@eslint/eslintrc/dist/eslintrc.cjs not supported. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Outside diff range and nitpick comments (4)
packages/configure/src/utils/authenticated.client.ts (2)
5-9: Consider strengthening the environment variable validation.While the removal of the localhost default aligns with the PR objectives, consider these improvements:
- Use strict equality (
===) for the null check- Add TypeScript type safety for the environment variable
- Consider moving the validation to a function that's called at runtime rather than at module level
-const FLATFILE_API_URL = process.env.AGENT_INTERNAL_URL - -if (FLATFILE_API_URL == null) { - throw new Error('AGENT_INTERNAL_URL must be set in the environment') -} +const validateApiUrl = (): string => { + const apiUrl = process.env.AGENT_INTERNAL_URL + if (apiUrl === null || apiUrl === undefined) { + throw new Error('AGENT_INTERNAL_URL must be set in the environment') + } + return apiUrl +} + +const FLATFILE_API_URL = validateApiUrl()
2-3: Address the TODO comment regarding conditional imports.The TODO comment suggests that the environment detection logic is pending. This could affect how
AGENT_INTERNAL_URLis handled in different contexts (NodeVM vs Browser).Would you like me to help create an issue to track the implementation of environment-specific handling?
packages/listener/src/events/authenticated.client.ts (2)
10-14: LGTM! Consider enhancing the error message.The implementation correctly removes the default localhost value and adds appropriate validation, aligning with the PR objectives.
Consider making the error message more descriptive:
- throw new Error('AGENT_INTERNAL_URL must be set in the environment') + throw new Error('AGENT_INTERNAL_URL environment variable must be set to a valid URL')
Line range hint
19-23: Simplify _apiUrl assignment and add URL validation.The current _apiUrl assignment logic is unnecessarily complex and lacks URL validation.
Consider this improved implementation:
- this._apiUrl = - apiUrl || FLATFILE_API_URL - ? ensureSingleTrailingSlash(apiUrl || FLATFILE_API_URL) - : undefined + const finalUrl = apiUrl || FLATFILE_API_URL; + try { + new URL(finalUrl); // Validate URL format + this._apiUrl = ensureSingleTrailingSlash(finalUrl); + } catch (e) { + throw new Error(`Invalid URL provided: ${finalUrl}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/configure/src/utils/authenticated.client.ts(1 hunks)packages/listener/src/events/authenticated.client.ts(1 hunks)
🔇 Additional comments (2)
packages/configure/src/utils/authenticated.client.ts (1)
Line range hint 13-65: Verify error handling and URL validation in client methods.
The removal of the default URL value introduces several potential edge cases that should be handled:
- The api getter and fetch method might throw errors after class instantiation
- URL concatenation in fetch method lacks validation
- Response handling assumes JSON data without proper error handling
Let's verify the usage patterns:
Consider these improvements:
- Validate URL format during initialization
- Add proper error handling for non-JSON responses
- Consider using URL.resolve for safer URL concatenation
- Add retry logic for transient failures
Would you like me to provide a code example implementing these improvements?
packages/listener/src/events/authenticated.client.ts (1)
Line range hint 25-48: Verify error handling in fetch method.
The fetch method assumes _apiUrl is defined and valid, but there's no explicit check. This could lead to runtime errors if initialization fails.
Let's verify the fetch method usage across the codebase:
bangarang
force-pushed
the
chore/remove-localhost-url
branch
from
c13b7ec to
90702e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/listener/src/events/authenticated.client.ts (3)
10-14: Enhance error handling message and nullish check.The implementation correctly enforces the AGENT_INTERNAL_URL requirement as per PR objectives. Consider these improvements:
const FLATFILE_API_URL = CrossEnvConfig.get('AGENT_INTERNAL_URL') -if (FLATFILE_API_URL == null) { - throw new Error('AGENT_INTERNAL_URL must be set in the environment') +if (FLATFILE_API_URL === null || FLATFILE_API_URL === undefined) { + throw new Error('AGENT_INTERNAL_URL environment variable must be set. Please configure this value before initializing the client.') }
Line range hint
19-23: Simplify apiUrl assignment logic and prevent undefined values.The current assignment logic is complex and could potentially set
this._apiUrlto undefined. This could cause runtime errors in the fetch method.- this._apiUrl = - apiUrl || FLATFILE_API_URL - ? ensureSingleTrailingSlash(apiUrl || FLATFILE_API_URL) - : undefined + this._apiUrl = ensureSingleTrailingSlash(apiUrl || FLATFILE_API_URL)
Line range hint
33-34: Add URL validation in fetch method.The fetch method should validate
this._apiUrlbefore constructing the fetchUrl to prevent potential runtime errors.+ if (!this._apiUrl) { + throw new Error('API URL is not configured. Ensure the client is properly initialized.') + } const fetchUrl = this._apiUrl + url
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/configure/src/utils/authenticated.client.ts(1 hunks)packages/listener/src/events/authenticated.client.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/configure/src/utils/authenticated.client.ts
🔇 Additional comments (1)
packages/listener/src/events/authenticated.client.ts (1)
10-10: Verify AGENT_INTERNAL_URL usage across the codebase.
Let's ensure consistent handling of AGENT_INTERNAL_URL across the codebase.
✅ Verification successful
Based on the search results, I can now generate the final response:
AGENT_INTERNAL_URL usage is consistent across the codebase
The environment variable is used consistently in three packages:
packages/listener/src/events/authenticated.client.ts: Uses CrossEnvConfig.get()packages/configure/src/utils/authenticated.client.ts: Uses process.env directlypackages/cli/src/shared/utils/polling.ts: Uses as fallback with platform URLpackages/cli/src/x/actions/develop.action.ts: Sets the environment variable
The change to use CrossEnvConfig.get() in the listener package is an improvement in configuration management and doesn't conflict with other usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of AGENT_INTERNAL_URL
echo "Searching for AGENT_INTERNAL_URL usage:"
rg "AGENT_INTERNAL_URL" -A 3
# Search for potential test files that might need updates
echo -e "\nSearching for related test files:"
fd -e test.ts -e spec.ts authenticated.client
Length of output: 1986
Head branch was pushed to by a user without write access
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
packages/listener/src/flatfile.listener.spec.ts (1)
10-11: Consider adding global test environment setupSince all tests require
AGENT_INTERNAL_URLto be set, consider moving the environment setup to a global test setup file orbeforeAllblock to maintain test isolation while ensuring consistent test environment.Example setup in a
jest.setup.js:// jest.setup.js beforeAll(() => { process.env.AGENT_INTERNAL_URL = 'test_url'; }); afterAll(() => { delete process.env.AGENT_INTERNAL_URL; });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/listener/src/events/event.handler.spec.ts(1 hunks)packages/listener/src/flatfile.listener.spec.ts(1 hunks)
🔇 Additional comments (2)
packages/listener/src/events/event.handler.spec.ts (1)
1-1: LGTM! Import statement is appropriate.
The addition of CrossEnvConfig import provides consistent environment variable handling across the codebase.
packages/listener/src/flatfile.listener.spec.ts (1)
1-1: Verify test coverage for environment variable handling
Let's ensure we have adequate test coverage for the new environment variable validation logic.
✅ Verification successful
Environment variable handling is adequately tested
The test file includes proper environment variable setup and validation:
- Checks and sets
AGENT_INTERNAL_URLif not present - Contains comprehensive test coverage for the listener functionality
- Includes extensive test cases for event triggering and filtering scenarios
The test suite demonstrates thorough coverage with:
- Basic event listener tests
- Namespaced event handling
- Array-based event handling
- Wildcard event patterns
- Multiple filtering scenarios
- Nested object validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage and related environment variable handling
# Look for other tests that might need similar environment validation
rg -l "AGENT_INTERNAL_URL" --type ts --type test
# Check for any environment setup in test files
rg "process\.env" --type ts --type test
# Look for related configuration handling
ast-grep --pattern 'CrossEnvConfig.get("AGENT_INTERNAL_URL")'
Length of output: 212
Script:
#!/bin/bash
# Description: Check test coverage and related environment variable handling with corrected commands
# Look for other tests that might need similar environment validation
rg -l "AGENT_INTERNAL_URL" "*.spec.ts" "*.test.ts"
# Check for any environment setup in test files
rg "process\.env" "*.spec.ts" "*.test.ts"
# Find all test files related to environment configuration
rg -l "CrossEnvConfig" "*.spec.ts" "*.test.ts"
# Look for test cases in the specific file
cat "packages/listener/src/flatfile.listener.spec.ts"
Length of output: 5873
| if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) { | ||
| process.env.AGENT_INTERNAL_URL = 'agent_internal_url' | ||
| } |
There was a problem hiding this comment.
Test setup needs revision to align with PR objectives.
The current test setup automatically sets a default value for AGENT_INTERNAL_URL, which contradicts the PR's objective of requiring explicit environment variable definition. According to the PR objectives, the application should throw an error when AGENT_INTERNAL_URL is not set.
Consider these improvements:
- Remove the default value assignment
- Add test cases to verify error handling when
AGENT_INTERNAL_URLis not set - Add test cases to verify successful operation with a valid URL
Here's a suggested approach:
- if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
- process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
- }
+ describe('AGENT_INTERNAL_URL validation', () => {
+ const originalEnv = process.env.AGENT_INTERNAL_URL;
+
+ afterEach(() => {
+ process.env.AGENT_INTERNAL_URL = originalEnv;
+ });
+
+ test('throws error when AGENT_INTERNAL_URL is not set', () => {
+ delete process.env.AGENT_INTERNAL_URL;
+ expect(() => new EventHandler()).toThrow('AGENT_INTERNAL_URL must be set in the environment');
+ });
+
+ test('creates handler successfully with valid AGENT_INTERNAL_URL', () => {
+ process.env.AGENT_INTERNAL_URL = 'http://test-agent:3000';
+ expect(() => new EventHandler()).not.toThrow();
+ });
+ });Committable suggestion skipped: line range outside the PR's diff.
| @@ -1,8 +1,13 @@ | |||
| import { CrossEnvConfig } from '@flatfile/cross-env-config' | |||
There was a problem hiding this comment.
Test setup contradicts PR objectives
The current implementation sets a default value for AGENT_INTERNAL_URL when null, which contradicts the PR objectives stating that the application should throw an error when this variable is not set.
Consider replacing this with tests that verify both success and error cases:
-if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
- process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
-}
+describe('AGENT_INTERNAL_URL validation', () => {
+ const originalEnv = process.env.AGENT_INTERNAL_URL;
+
+ afterEach(() => {
+ process.env.AGENT_INTERNAL_URL = originalEnv;
+ });
+
+ test('throws error when AGENT_INTERNAL_URL is not set', () => {
+ delete process.env.AGENT_INTERNAL_URL;
+ expect(() => {
+ FlatfileListener.create((c) => {
+ c.on('foo', () => {});
+ });
+ }).toThrow('AGENT_INTERNAL_URL must be set in the environment');
+ });
+
+ test('creates listener successfully when AGENT_INTERNAL_URL is set', () => {
+ process.env.AGENT_INTERNAL_URL = 'test_url';
+ expect(() => {
+ FlatfileListener.create((c) => {
+ c.on('foo', () => {});
+ });
+ }).not.toThrow();
+ });
+});Also applies to: 7-9
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/listener/src/events/event.handler.spec.ts(2 hunks)packages/listener/src/flatfile.listener.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/listener/src/events/event.handler.spec.ts
🔇 Additional comments (1)
packages/listener/src/flatfile.listener.spec.ts (1)
7-9:
Remove default value setting to align with PR objectives
Setting a default value for AGENT_INTERNAL_URL contradicts the PR's goal of ensuring the variable must be explicitly set. This could mask issues in tests where the variable is not properly configured.
Remove these lines and ensure each test properly sets up its environment variables:
- if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
- process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
- }| describe('AGENT_INTERNAL_URL', () => { | ||
| test('throws error when not set', () => { | ||
| process.env.AGENT_INTERNAL_URL = undefined | ||
| expect(() => { | ||
| FlatfileListener.create((c) => { | ||
| c.on('foo', () => {}) | ||
| }) | ||
| }).toThrow('AGENT_INTERNAL_URL must be set in the environment') | ||
| }) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage and improve test isolation
While testing the error case is good, the test suite needs improvements:
- Missing test for the success case when AGENT_INTERNAL_URL is properly set
- No cleanup of environment variables after tests
- Using
undefinedinstead of actually removing the variable
Consider this improved implementation:
describe('AGENT_INTERNAL_URL', () => {
+ const originalEnv = process.env.AGENT_INTERNAL_URL;
+
+ afterEach(() => {
+ process.env.AGENT_INTERNAL_URL = originalEnv;
+ });
+
test('throws error when not set', () => {
- process.env.AGENT_INTERNAL_URL = undefined
+ delete process.env.AGENT_INTERNAL_URL;
expect(() => {
FlatfileListener.create((c) => {
c.on('foo', () => {})
})
}).toThrow('AGENT_INTERNAL_URL must be set in the environment')
})
+
+ test('creates listener successfully when AGENT_INTERNAL_URL is set', () => {
+ process.env.AGENT_INTERNAL_URL = 'test_url';
+ expect(() => {
+ FlatfileListener.create((c) => {
+ c.on('foo', () => {})
+ })
+ }).not.toThrow();
+ });
})📝 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.
| describe('AGENT_INTERNAL_URL', () => { | |
| test('throws error when not set', () => { | |
| process.env.AGENT_INTERNAL_URL = undefined | |
| expect(() => { | |
| FlatfileListener.create((c) => { | |
| c.on('foo', () => {}) | |
| }) | |
| }).toThrow('AGENT_INTERNAL_URL must be set in the environment') | |
| }) | |
| }) | |
| describe('AGENT_INTERNAL_URL', () => { | |
| const originalEnv = process.env.AGENT_INTERNAL_URL; | |
| afterEach(() => { | |
| process.env.AGENT_INTERNAL_URL = originalEnv; | |
| }); | |
| test('throws error when not set', () => { | |
| delete process.env.AGENT_INTERNAL_URL; | |
| expect(() => { | |
| FlatfileListener.create((c) => { | |
| c.on('foo', () => {}) | |
| }) | |
| }).toThrow('AGENT_INTERNAL_URL must be set in the environment') | |
| }) | |
| test('creates listener successfully when AGENT_INTERNAL_URL is set', () => { | |
| process.env.AGENT_INTERNAL_URL = 'test_url'; | |
| expect(() => { | |
| FlatfileListener.create((c) => { | |
| c.on('foo', () => {}) | |
| }) | |
| }).not.toThrow(); | |
| }); | |
| }) |
|
@josephemswiler I moved this to an internal PR sO I could push code to it too without writing to your repo! |
3 participants
Please explain how to summarize this PR for the Changelog:
The
AGENT_INTERNAL_URLenvironment variable is being exposed in flatfile's compiled JS with a value of"http://localhost:3000". This change removes that value and returns an error if theAGENT_INTERNAL_URLenvironment variable is not set.Tell code reviewer how and what to test:
In a test environment, set the
AGENT_INTERNAL_URLenvironment variable to a nullish value. Expect the application to throw the errorAGENT_INTERNAL_URL must be set in the environment.In a test environment, set the
AGENT_INTERNAL_URLenvironment variable to a valid value. Expect the application not to throw an error.