[wip] configure frontend enviornment variables #141
Conversation
WalkthroughThe pull request introduces changes to the frontend API configuration by modifying how the base API URL is determined. The Changes
Sequence DiagramsequenceDiagram
participant App as Frontend Application
participant Config as API Config Module
participant API as API Module
App->>Config: Determine environment
Config-->>App: Return appropriate API URL
App->>API: Initialize with base URL
API->>Server: Make API requests
Possibly Related PRs
Suggested Reviewers
Poem
🪧 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 (
|
ken-zlai
changed the title
![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 comments (2)
frontend/src/lib/api/api.ts (1)
Line range hint
13-23: Enhance error handling in send functionThe current error handling doesn't provide enough context about API failures.
Consider this enhancement:
async function send({ method, path }: { method: string; path: string }) { const opts = { method, headers: {} }; + try { + const res = await fetch(`${base}/${path}`, opts); + if (res.ok) { + const text = await res.text(); + return text ? JSON.parse(text) : {}; + } - error(res.status); + const errorText = await res.text(); + error(res.status, { + message: `API request failed: ${errorText}`, + code: res.status + }); + } catch (e) { + error(500, { + message: `Network error: ${e.message}`, + code: 500 + }); + } }docker-init/compose.yaml (1)
Line range hint
89-106: Add required environment variables for frontend serviceThe frontend service is missing the necessary environment variables for API configuration.
Add the following to the frontend service configuration:
frontend: build: context: .. dockerfile: docker-init/frontend/Dockerfile depends_on: - app + environment: + - PUBLIC_API_EXTERNAL_URL=http://localhost:9000 + - PUBLIC_API_BASE_URL=http://app:9000 ports: - "3000:3000" healthcheck: test: ["CMD", "curl", "-f", "http://localhost:3000"] interval: 30s timeout: 10s retries: 3 start_period: 40s
🧹 Nitpick comments (1)
frontend/src/lib/api/api.ts (1)
10-10: Consider making apiBaseUrl more dynamicSetting the base URL as a module-level constant prevents it from updating if environment variables change during runtime.
Consider this approach:
-const apiBaseUrl = getApiBaseUrl(); - -const base = `${apiBaseUrl}/api/v1`; async function send({ method, path }: { method: string; path: string }) { + const base = `${getApiBaseUrl()}/api/v1`; const opts = { method, headers: {} };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
docker-init/compose.yaml(1 hunks)frontend/src/lib/api/api-config.ts(1 hunks)frontend/src/lib/api/api.ts(1 hunks)
🔇 Additional comments (1)
docker-init/compose.yaml (1)
Line range hint 89-106: Verify frontend healthcheck endpoint
The healthcheck is testing the root endpoint, but we should verify if a more specific health endpoint exists.
✅ Verification successful
Let me check the content of these files to see if there are any specific health endpoints defined.
Let me check the actual frontend routes and components to see if there's any health-related endpoint implementation.
Let me check the root directory for any health check configurations in the frontend service.
Let me check the actual healthcheck configuration in the docker-compose file to understand what's currently being used.
Frontend healthcheck is using a basic endpoint test which is sufficient
The current healthcheck configuration for the frontend service is testing the root endpoint (http://localhost:3000) which is appropriate for this use case because:
- It verifies that the HTTP server is up and responding
- It's a lightweight check that doesn't put unnecessary load on the service
- It follows common Docker healthcheck practices for frontend services
No changes are needed to the current healthcheck configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for health-related endpoints in the frontend codebase
rg -l 'health|ping|status' frontend/src/
Length of output: 223
Script:
#!/bin/bash
# Check the content of api files for health-related endpoints
rg -A 5 'health|ping|status' frontend/src/lib/api/api.ts
Length of output: 158
Script:
#!/bin/bash
# Search for route definitions and health-related components
rg -A 5 "route|Route" frontend/src/
Length of output: 1694
Script:
#!/bin/bash
# Look for frontend service configuration files
fd -t f "frontend|health" . --exec grep -l "health\|healthcheck\|ping"
Length of output: 72
Script:
#!/bin/bash
# Check the healthcheck configuration in the docker-compose file
rg -A 5 "healthcheck" docker-init/compose.yaml
Length of output: 567
| const API_CONFIGS = { | ||
| dev: { | ||
| client: 'http://localhost:9000', | ||
| server: 'http://localhost:9000' | ||
| }, | ||
| prod: { | ||
| client: env.PUBLIC_API_EXTERNAL_URL ?? 'http://localhost:9000', | ||
| server: env.PUBLIC_API_BASE_URL ?? 'http://app:9000' | ||
| } | ||
| }; |
There was a problem hiding this comment.
Review environment variable configuration and default values
The configuration structure has several concerns:
- Default URLs should not point to localhost in production
- The server URL in production points to 'http://' which isn't secure for production use
Consider these changes:
const API_CONFIGS = {
dev: {
client: 'http://localhost:9000',
server: 'http://localhost:9000'
},
prod: {
- client: env.PUBLIC_API_EXTERNAL_URL ?? 'http://localhost:9000',
- server: env.PUBLIC_API_BASE_URL ?? 'http://app:9000'
+ client: env.PUBLIC_API_EXTERNAL_URL ?? '/api',
+ server: env.PUBLIC_API_BASE_URL ?? 'https://api.chronon.ai'
}
};Committable suggestion skipped: line range outside the PR's diff.
| export function getApiBaseUrl() { | ||
| const deployEnv = import.meta.env.PROD ? 'prod' : 'dev'; | ||
| return browser ? API_CONFIGS[deployEnv].client : API_CONFIGS[deployEnv].server; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for missing environment variables in production
The current implementation silently falls back to default values in production, which could lead to unexpected behavior.
Consider adding validation:
export function getApiBaseUrl() {
const deployEnv = import.meta.env.PROD ? 'prod' : 'dev';
+ if (deployEnv === 'prod') {
+ if (!env.PUBLIC_API_EXTERNAL_URL || !env.PUBLIC_API_BASE_URL) {
+ console.warn('Missing API configuration environment variables in production');
+ }
+ }
return browser ? API_CONFIGS[deployEnv].client : API_CONFIGS[deployEnv].server;
}Committable suggestion skipped: line range outside the PR's diff.
| client: env.PUBLIC_API_EXTERNAL_URL ?? 'http://localhost:9000', | ||
| server: env.PUBLIC_API_BASE_URL ?? 'http://app:9000' |
There was a problem hiding this comment.
Just to make sure I understand,
PUBLIC_API_EXTERNAL_URLwill point to the public DNS (ex.http://app.zipline.ai:9000, or AWS EC2 host/etc)PUBLIC_API_BASE_URLwill point to the internal DNS (ex.http://app:9000) based on docker compose.yml)
but they are both pointing at the same API/backend?
I wonder if we could either:
- Only have a single URL per environment (dev/prod), but always fully qualifying it (always use
app.zipline.ai,{instance}.us-west-1.elb.amazonaws.com, etc) instead of using the internalapp:9000for the server - Proxy requests through the frontend node server,
so/apigoes tolocalhost:9000orapp:9000based on environment, but to the client it's always/api` (removes browser CORS issues as well)
There was a problem hiding this comment.
Basically following the last note on the SvelteKit FAQ.
Something like:
src/routes/api/[...path]/+server.js
import { env } from '$env/dynamic/private'; // can be private (not `PUBLIC_` prefixed) since no longer needed client-side
import type { RequestHandler } from './$types';
export const GET: RequestHandler = ({ params, url }) => {
return fetch(`https://${env.API_BASE_URL}/${params.path + url.search}`);
};
export const POST: RequestHandler = ({ params, url }) => {
return fetch(`https://${env.API_BASE_URL}/${params.path + url.search}`);
};
// ... other methods as needed
|
We chose to use #143 as a solution instead |
Summary
I've made some updates to the environment variable configuration. Everything seems to work locally at
http://localhost:5173/andhttp://localhost:3000/, but I'm not sure if it will function correctly in the deployed environment.The goal is to ensure the app calls the correct APIs both locally (using the URLs above) and in production current version. Currently, the issue is that client-side API calls don't have a public-facing endpoint to connect to.
@sean-zlai, could you take a look? Configuring environment variables in Svelte is still new to me. From what I understand, the variables should be exposed from this configuration once prefixed with
PUBLIC_. Also linking Chewy's PR, which should add the load balancer and public-facing API URL for the client.Checklist
Summary by CodeRabbit
New Features
Bug Fixes
frontendservice by removing theAPI_BASE_URLvariable.Chores