Chore/remove localhost url #228
Conversation
WalkthroughThe changes in this pull request focus on enforcing the requirement for the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant Environment
Client->>Environment: Check AGENT_INTERNAL_URL
alt AGENT_INTERNAL_URL is set
Environment-->>Client: Provide AGENT_INTERNAL_URL
Client->>Server: Fetch data using FLATFILE_API_URL
Server-->>Client: Return data
else AGENT_INTERNAL_URL is not set
Environment-->>Client: Throw error
end
Possibly related PRs
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/authenticated.client.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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🔇 Additional comments (1)packages/react/src/components/_tests_/FlatfileProvider.spec.tsx (1)
The change from an empty string to the production URL aligns with the PR objective of removing localhost URLs. Let's verify URL consistency across test files: ✅ Verification successfulURL pattern is consistent across test files The verification shows that the production URL pattern
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any inconsistent API URLs in test files
# Expected: All test files should use the same API URL pattern
# Search for API URLs in test files
rg -g '*test*' -g '*spec*' 'platform\.flatfile\.com/api'
# Search for potential localhost URLs that might have been missed
rg -g '*test*' -g '*spec*' 'localhost:[0-9]+'
Length of output: 330 🪧 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: 2
🧹 Outside diff range and nitpick comments (11)
packages/configure/src/utils/authenticated.client.ts (4)
5-9: Enhance environment variable validationWhile removing the localhost URL is good, the validation could be more robust.
Consider this enhanced validation:
-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 FLATFILE_API_URL = process.env.AGENT_INTERNAL_URL?.trim() + +if (!FLATFILE_API_URL) { + throw new Error('AGENT_INTERNAL_URL environment variable must be set with a valid URL') +} + +try { + new URL(FLATFILE_API_URL) +} catch (e) { + throw new Error(`Invalid URL provided in AGENT_INTERNAL_URL: ${FLATFILE_API_URL}`) +}
Line range hint
22-31: Improve authentication token handlingThe fallback to '...' for missing bearer tokens could mask configuration issues in production.
Consider this safer approach:
const ClientConfig = new Configuration({ basePath: `${FLATFILE_API_URL}/v1`, fetchApi: fetch, - accessToken: process.env.FLATFILE_BEARER_TOKEN, + accessToken: this.validateToken(), headers: { - Authorization: `Bearer ${process.env.FLATFILE_BEARER_TOKEN || '...'}`, + Authorization: `Bearer ${this.validateToken()}`, 'x-disable-hooks': 'true', }, })Add this helper method:
private validateToken(): string { const token = process.env.FLATFILE_BEARER_TOKEN?.trim() if (!token) { throw new Error('FLATFILE_BEARER_TOKEN environment variable must be set') } return token }
Line range hint
34-57: Multiple critical issues in fetch implementationSeveral issues need to be addressed in the fetch method:
- The caching logic is incorrect and will always return the same cached response:
- if (this._fetch) { - return this._fetch - }
- URL concatenation is unsafe:
- const fetchUrl = FLATFILE_API_URL + '/' + url + const fetchUrl = new URL(url, FLATFILE_API_URL).toString()
- Authorization header has incorrect nullish coalescing:
- Authorization: - `Bearer ${process.env.FLATFILE_BEARER_TOKEN}` ?? `Bearer ...`, + Authorization: `Bearer ${this.validateToken()}`,
- Error handling is insufficient:
return fetch(fetchUrl, { method: 'GET', ...options, headers, }) - .then((resp) => resp.json()) - .then((resp) => resp.data) + .then(async (resp) => { + if (!resp.ok) { + const error = await resp.text() + throw new Error(`API request failed: ${resp.status} ${error}`) + } + const data = await resp.json() + return data.data + })
Localhost references found in CLI and legacy utilities
The codebase still contains localhost references in several files, primarily in CLI and legacy utilities:
packages/cli/src/x/actions/quickstart.action.ts: Uses hardcodedhttp://localhost:6789packages/cli/src/legacy/utilities/embed.url.ts: Containshttp://localhost:8080referencespackages/cli/src/legacy/utilities/schema.url.ts: Containshttp://localhost:8080referencesapps/sandbox/package.json: Containshttp://localhost:3000reference🔗 Analysis chain
Line range hint
1-57: Verify complete removal of localhost referencesLet's ensure no localhost references remain in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining localhost references in the codebase echo "Searching for localhost references..." rg -i "localhost" --type-add 'config:*.{json,js,ts,env}' -t config # Search for hardcoded URLs that might need similar treatment echo "Searching for other hardcoded URLs..." rg -i "https?://" --type-add 'config:*.{json,js,ts,env}' -t configLength of output: 128497
packages/listener/src/events/authenticated.client.ts (5)
12-14: Enhance error message for missing AGENT_INTERNAL_URLThe error message could be more descriptive to help developers understand the purpose of this environment variable and how to set it correctly.
- throw new Error('AGENT_INTERNAL_URL must be set in the environment') + throw new Error('AGENT_INTERNAL_URL environment variable must be set to specify the internal API endpoint')
Line range hint
20-24: Simplify _apiUrl assignment logicThe current assignment logic using the ternary operator is unnecessarily complex and could be simplified since we now validate FLATFILE_API_URL earlier.
- this._apiUrl = - apiUrl || FLATFILE_API_URL - ? ensureSingleTrailingSlash(apiUrl || FLATFILE_API_URL) - : undefined + this._apiUrl = ensureSingleTrailingSlash(apiUrl || FLATFILE_API_URL)
Line range hint
26-26: Add type safety for fetch optionsThe
optionsparameter is typed asany, which bypasses TypeScript's type safety. Consider creating an interface for the options parameter.- async fetch(url: string, options?: any) { + interface FetchOptions { + method?: string; + headers?: Record<string, string>; + data?: any; + } + async fetch(url: string, options?: FetchOptions) {
Line range hint
47-49: Improve error handling in fetch methodThe catch block only logs the error without proper error propagation. This could mask issues and make debugging harder.
- } catch (err) { - console.log('event.fetch error: ', err) + } catch (err) { + console.error('Failed to fetch from API:', err); + throw err;
Line range hint
54-54: Add deprecation timeline for setVariablesThe
setVariablesmethod is marked as deprecated, but there's no indication of when it will be removed. Consider adding a timeline and migration guide in the deprecation notice.- * @deprecated use @flatfile/cross-env-config instead + * @deprecated Since version X.Y.Z. Will be removed in version X+1.0.0. + * Use @flatfile/cross-env-config instead. + * Migration guide: [link to documentation]packages/listener/src/events/event.handler.spec.ts (1)
95-102: Improve test isolation for AGENT_INTERNAL_URL validationThe test correctly verifies the error case, but modifying
process.envglobally could affect other tests.Consider using a more isolated approach:
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 expect(() => new EventHandler()).toThrow( 'AGENT_INTERNAL_URL must be set in the environment' ) }) + + test('succeeds when properly set', () => { + process.env.AGENT_INTERNAL_URL = 'http://test-api.example.com' + expect(() => new EventHandler()).not.toThrow() + }) })packages/listener/src/flatfile.listener.spec.ts (1)
159-166: Enhance test coverage and environment handlingWhile the test correctly verifies the error case, it could be improved in several ways:
- Add positive test case
- Verify exact error message format
- Restore environment state after test
Consider expanding the test suite:
describe('AGENT_INTERNAL_URL', () => { + const originalUrl = process.env.AGENT_INTERNAL_URL; + + afterEach(() => { + process.env.AGENT_INTERNAL_URL = originalUrl; + }); + test('throws error when not set', () => { delete process.env.AGENT_INTERNAL_URL; - expect(() => new FlatfileListener()).toThrow( - 'AGENT_INTERNAL_URL must be set in the environment' - ) + expect(() => new FlatfileListener()).toThrow(new Error( + 'AGENT_INTERNAL_URL must be set in the environment' + )); }); + + test('creates instance successfully when AGENT_INTERNAL_URL is set', () => { + process.env.AGENT_INTERNAL_URL = 'test-url'; + expect(() => new FlatfileListener()).not.toThrow(); + }); });🧰 Tools
🪛 Biome (1.9.4)
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
packages/configure/src/utils/authenticated.client.ts(1 hunks)packages/listener/src/events/authenticated.client.ts(1 hunks)packages/listener/src/events/event.handler.spec.ts(2 hunks)packages/listener/src/flatfile.listener.spec.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/listener/src/flatfile.listener.spec.ts
[error] 161-161: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (3)
packages/listener/src/events/authenticated.client.ts (1)
10-14: Verify removal of localhost URLs across the codebase
Let's ensure there are no remaining hardcoded localhost URLs in the codebase.
✅ Verification successful
No issues with localhost URL usage in production code
The localhost URLs found in the codebase are appropriately used in development and testing contexts:
- In
apps/sandbox/package.json: Used for local development command - In
packages/cli/src/legacy/utilities/*.ts: Used for local development URL construction with proper hostname checks - In
packages/cli/src/x/actions/quickstart.action.ts: Has a TODO comment indicating it's pending configuration for Local vs Prod
The change to enforce AGENT_INTERNAL_URL environment variable is consistent with the codebase's approach to URL configuration, as evidenced by:
- Similar environment variable checks in test files
- Proper fallback patterns in authentication clients
- Consistent error messaging for missing configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining localhost URLs in the codebase
rg -i "localhost|127\.0\.0\.1" --type-add 'config:*.{json,js,ts,env}' -t config
# Search for any default URL assignments that might need similar environment variable enforcement
ast-grep --pattern 'FLATFILE_API_URL = $_'
Length of output: 815
Script:
#!/bin/bash
# Let's check the context of these localhost URLs to understand if they are for development/testing
rg -B 3 -A 3 "localhost|127\.0\.0\.1" packages/cli/src/legacy/utilities/schema.url.ts packages/cli/src/legacy/utilities/embed.url.ts packages/cli/src/x/actions/quickstart.action.ts
# Also check for any environment variable usage patterns similar to AGENT_INTERNAL_URL
rg -i "internal.*url|api.*url" --type ts
Length of output: 22051
packages/listener/src/events/event.handler.spec.ts (1)
1-1: 🛠️ Refactor suggestion
Reconsider the default AGENT_INTERNAL_URL in tests
Setting a default value for AGENT_INTERNAL_URL in the test setup while enforcing its requirement elsewhere could mask potential issues. Additionally, the value 'agent_internal_url' is not a valid URL format.
Let's check if other test files follow the same pattern:
Consider these alternatives:
- Remove the default value and update individual tests to set appropriate URLs
- If a default is needed, use a valid URL format like
'http://localhost:3000'
- if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
- process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
- }
+ beforeEach(() => {
+ process.env.AGENT_INTERNAL_URL = 'http://test-api.example.com'
+ })
+
+ afterEach(() => {
+ delete process.env.AGENT_INTERNAL_URL
+ })Also applies to: 7-9
packages/listener/src/flatfile.listener.spec.ts (1)
Line range hint 1-166: Verify removal of localhost URLs
Let's ensure no localhost URLs are being used in the test values or mocks.
✅ Verification successful
No localhost URLs found in the codebase
The verification confirms that there are no localhost or hardcoded URLs in the test files. The only URL references are:
- Environment variable
AGENT_INTERNAL_URLwith a placeholder value 'agent_internal_url' - A single documentation comment mentioning HTTP2 in a technical context
- Error message strings for HTTP status handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any localhost URLs in the test file and related files
rg -i "localhost|127\.0\.0\.1" packages/listener/src/
Length of output: 55
Script:
#!/bin/bash
# Let's search for any URLs or IP addresses in the test files to ensure no hardcoded values
rg -i "http://|https://|:\/\/|\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}" packages/listener/src/
Length of output: 232
Script:
#!/bin/bash
# Search for URL patterns with corrected regex
rg -i "http|https|://|agent_.*_url" packages/listener/src/
Length of output: 1562
| const FLATFILE_API_URL = | ||
| CrossEnvConfig.get('AGENT_INTERNAL_URL') || 'http://localhost:3000' | ||
| const FLATFILE_API_URL = CrossEnvConfig.get('AGENT_INTERNAL_URL') | ||
| console.log('FLATFILE_API_URL', FLATFILE_API_URL) |
There was a problem hiding this comment.
Remove console.log statement
Production code should not contain console.log statements, especially ones that might expose sensitive configuration information. Consider using a proper logging system if debugging is needed.
- console.log('FLATFILE_API_URL', FLATFILE_API_URL)📝 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.
| console.log('FLATFILE_API_URL', FLATFILE_API_URL) |
| if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) { | ||
| process.env.AGENT_INTERNAL_URL = 'agent_internal_url' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move environment setup into appropriate test lifecycle hooks
The environment variable setup should be moved into appropriate test lifecycle hooks (like beforeAll/afterAll) to ensure proper test isolation and cleanup.
Consider this approach:
- if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
- process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
- }
+ let originalAgentInternalUrl: string | undefined;
+
+ beforeAll(() => {
+ originalAgentInternalUrl = process.env.AGENT_INTERNAL_URL;
+ if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) {
+ process.env.AGENT_INTERNAL_URL = 'agent_internal_url'
+ }
+ });
+
+ afterAll(() => {
+ if (originalAgentInternalUrl === undefined) {
+ delete process.env.AGENT_INTERNAL_URL;
+ } else {
+ process.env.AGENT_INTERNAL_URL = originalAgentInternalUrl;
+ }
+ });📝 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.
| if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) { | |
| process.env.AGENT_INTERNAL_URL = 'agent_internal_url' | |
| } | |
| let originalAgentInternalUrl: string | undefined; | |
| beforeAll(() => { | |
| originalAgentInternalUrl = process.env.AGENT_INTERNAL_URL; | |
| if (CrossEnvConfig.get('AGENT_INTERNAL_URL') == null) { | |
| process.env.AGENT_INTERNAL_URL = 'agent_internal_url' | |
| } | |
| }); | |
| afterAll(() => { | |
| if (originalAgentInternalUrl === undefined) { | |
| delete process.env.AGENT_INTERNAL_URL; | |
| } else { | |
| process.env.AGENT_INTERNAL_URL = originalAgentInternalUrl; | |
| } | |
| }); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/listener/src/events/event.handler.spec.ts (1)
97-97: Consider using undefined assignment instead of deleteWhile functionally correct, using the delete operator can have performance implications.
- delete process.env.AGENT_INTERNAL_URL + process.env.AGENT_INTERNAL_URL = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 97-97: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/listener/src/events/authenticated.client.ts(1 hunks)packages/listener/src/events/event.handler.spec.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/listener/src/events/authenticated.client.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/listener/src/events/event.handler.spec.ts
[error] 97-97: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
packages/listener/src/events/event.handler.spec.ts (2)
95-102: LGTM! Good test coverage for environment variable requirement
The test suite properly verifies that EventHandler throws an error when AGENT_INTERNAL_URL is not set, which aligns with the PR's objective to enforce proper URL configuration.
🧰 Tools
🪛 Biome (1.9.4)
[error] 97-97: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
7-9: 🛠️ Refactor suggestion
Consider removing the default AGENT_INTERNAL_URL in tests
Setting a default value for AGENT_INTERNAL_URL in the test setup could mask issues where the environment variable is missing in production. Since we're enforcing this requirement, tests should reflect the same constraints.
Let's check if this pattern is consistent across other test files:
Consider moving the environment setup to a shared test helper and explicitly set/unset it in relevant test cases:
// test-helpers.ts
export const withEnvVar = (key: string, value: string, fn: () => void) => {
const original = process.env[key];
process.env[key] = value;
fn();
if (original === undefined) {
delete process.env[key];
} else {
process.env[key] = original;
}
};
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
packages/configure/src/utils/authenticated.client.ts (2)
5-5: LGTM! Consider adding type safety for environment variables.The removal of the hardcoded localhost URL is a good security practice.
Consider adding type safety:
-const FLATFILE_API_URL = process.env.AGENT_INTERNAL_URL +const FLATFILE_API_URL: string | undefined = process.env.AGENT_INTERNAL_URL
15-17: Consider extracting the environment variable validation.The null check is good, but it's duplicated in the fetch method. Consider extracting this validation to a private method.
+private validateEnvironment(): void { + if (FLATFILE_API_URL == null) { + throw new Error('AGENT_INTERNAL_URL must be set in the environment') + } +} get api(): DefaultApi { if (this._api) { return this._api } - if (FLATFILE_API_URL == null) { - throw new Error('AGENT_INTERNAL_URL must be set in the environment') - } + this.validateEnvironment() const ClientConfig = new Configuration({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.changeset/seven-years-knock.md(1 hunks)packages/configure/src/utils/authenticated.client.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- .changeset/seven-years-knock.md
🔇 Additional comments (1)
packages/configure/src/utils/authenticated.client.ts (1)
5-5: Verify environment variable documentation and remaining localhost references.
Let's ensure the AGENT_INTERNAL_URL requirement is properly documented and no localhost references remain.
✅ Verification successful
Environment variable is properly documented and used consistently
The AGENT_INTERNAL_URL environment variable is:
- Properly documented with a default value in
.env.defaults: "https://platform.flatfile.com/api" - Consistently used across the codebase with appropriate error handling
- Has proper error messages when not set: "AGENT_INTERNAL_URL must be set in the environment"
Regarding localhost references:
- The remaining localhost references are only in CLI-related development and testing code, which is appropriate for local development scenarios
- No production code contains hardcoded localhost references
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining localhost references in the codebase
echo "Checking for remaining localhost references..."
rg -i "localhost" --type ts
# Check if AGENT_INTERNAL_URL is documented in README or similar files
echo "Checking for AGENT_INTERNAL_URL documentation..."
rg -i "AGENT_INTERNAL_URL" -g "README*" -g "*.md"
Length of output: 866
Script:
#!/bin/bash
# Let's check for environment variable documentation in other common locations
echo "Checking for environment variable documentation in common config files..."
rg -i "AGENT_INTERNAL_URL" -g "*.env*" -g "config*" -g "*.example*"
# Let's also check the specific file and its surrounding context
echo "Checking the specific file context..."
rg -C 5 "AGENT_INTERNAL_URL" packages/configure/src/utils/authenticated.client.ts
# Check if there are any other usages of this environment variable
echo "Checking other usages of AGENT_INTERNAL_URL..."
rg "AGENT_INTERNAL_URL" --type ts
Length of output: 3456
| if (FLATFILE_API_URL == null) { | ||
| throw new Error('AGENT_INTERNAL_URL must be set in the environment') | ||
| } | ||
| const fetchUrl = FLATFILE_API_URL + '/' + url | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve URL construction and type safety.
A few suggestions to enhance this section:
- Use URL class for safer URL construction
- Add proper typing for the cached fetch response
- if (FLATFILE_API_URL == null) {
- throw new Error('AGENT_INTERNAL_URL must be set in the environment')
- }
- const fetchUrl = FLATFILE_API_URL + '/' + url
+ this.validateEnvironment()
+ const fetchUrl = new URL(url, FLATFILE_API_URL).toString()
- private _fetch?: any
+ private _fetch?: Promise<unknown>Committable suggestion skipped: line range outside the PR's diff.
Please explain how to summarize this PR for the Changelog:
Tell code reviewer how and what to test: