Fix proxy when specifying PROD=true#467
Conversation
WalkthroughHealth-check path changed to /api/health, API URL resolution now returns '' when PROD is true (ignoring VITE_API_URL), tests adjusted, Vite config adds allowedHosts and exposes import.meta.env.PROD, docker-compose and .env.example add a PROD variable. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App
participant Config as getApiUrl()
participant Env as env / import.meta.env
User->>App: Load app / perform API call
App->>Config: getApiUrl()
Config->>Env: Read PROD, VITE_API_URL, PORT, window.location
alt PROD === true
note right of Config #DDEBF7: Production path\n(ignore VITE_API_URL)
Config-->>App: return ''
else PROD !== true
alt VITE_API_URL set
Config-->>App: return VITE_API_URL
else
Config-->>App: return constructed URL (window.location + PORT)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type 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: 1
🧹 Nitpick comments (3)
archon-ui-main/src/config/api.ts (1)
15-19: Resolution order change is correct; consider naming clarityChecking PROD before VITE_API_URL achieves the intended “use proxy when PROD=true” behavior. One caution: you’re reusing the reserved Vite semantic flag name (PROD). That can be confusing since Vite already sets import.meta.env.PROD based on build mode.
If you want to avoid overloading semantics, rename the flag to VITE_USE_PROXY (or similar) and switch the check:
- if (import.meta.env.PROD) { + if (import.meta.env.VITE_USE_PROXY) { return ''; }See paired suggestion in vite.config.ts.
archon-ui-main/vite.config.ts (2)
283-283: Filter falsy allowedHosts entriesenv.HOST can be undefined; passing it through yields [undefined, 'localhost', '127.0.0.1'], which may trip type checks or cause noisy warnings.
- allowedHosts: [env.HOST, 'localhost', '127.0.0.1'], + allowedHosts: [env.HOST, 'localhost', '127.0.0.1'].filter(Boolean),
312-313: Refactor suggestion: Don’t overrideimport.meta.env.PROD, introduce a dedicated flagOverriding Vite’s built-in
import.meta.env.PRODcan lead to mismatches between Vite’s actual build mode and code that relies on it. Since we only see it used in:
archon-ui-main/vite.config.ts(lines 312–313)archon-ui-main/src/config/api.ts(theif (import.meta.env.PROD)check)you can safely introduce a new flag—e.g.
VITE_USE_PROXY—to drive proxy behavior:• In vite.config.ts:
- 'import.meta.env.PROD': env.PROD === 'true', + 'import.meta.env.VITE_USE_PROXY': env.PROD === 'true',• In src/config/api.ts:
- if (import.meta.env.PROD) { + if (import.meta.env.VITE_USE_PROXY) { return ''; }• Tests: we didn’t find any existing tests referencing
import.meta.env.PROD. If you add or update tests aroundgetApiUrl(), mockimport.meta.env.VITE_USE_PROXYinstead ofPROD.This change preserves Vite’s semantics for
import.meta.env.PRODwhile clearly signaling when you’re forcing proxy mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
archon-ui-main/src/components/layouts/MainLayout.tsx(2 hunks)archon-ui-main/src/config/api.ts(2 hunks)archon-ui-main/test/config/api.test.ts(2 hunks)archon-ui-main/vite.config.ts(2 hunks)docker-compose.yml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}
📄 CodeRabbit inference engine (CLAUDE.md)
{python/**/*.py,archon-ui-main/src/**/*.{ts,tsx,js,jsx}}: Remove dead code immediately; do not keep legacy/unused functions
Avoid comments that reference change history (e.g., LEGACY, CHANGED, REMOVED); keep comments focused on current functionality
Files:
archon-ui-main/src/components/layouts/MainLayout.tsxarchon-ui-main/src/config/api.ts
archon-ui-main/src/components/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in archon-ui-main/src/components/
Files:
archon-ui-main/src/components/layouts/MainLayout.tsx
archon-ui-main/test/**
📄 CodeRabbit inference engine (CLAUDE.md)
Place frontend tests under archon-ui-main/test/
Files:
archon-ui-main/test/config/api.test.ts
🧬 Code graph analysis (1)
archon-ui-main/src/components/layouts/MainLayout.tsx (1)
archon-ui-main/src/services/credentialsService.ts (1)
credentialsService(371-371)
🔇 Additional comments (5)
docker-compose.yml (1)
157-157: Healthcheck endpoints remain available at/health, so no Docker Compose changes are neededI’ve confirmed that both services continue to expose a
/healthendpoint (with/api/healthas an alias on the server), matching the existing Docker healthchecks.• Server service
–python/src/server/main.pydefines@app.get("/health")(line 234) and@app.get("/api/health")(line 259).
–python/Dockerfile.serverhealthcheck still callshttp://localhost:${ARCHON_SERVER_PORT}/health.• Agents service
–python/src/agents/server.pydefines@app.get("/health")(line 151).
–python/Dockerfile.agentshealthcheck still callshttp://localhost:${ARCHON_AGENTS_PORT}/health.Since
/healthis still supported, you can leave the- CMD .../healthchecks indocker-compose.ymlas-is.archon-ui-main/src/components/layouts/MainLayout.tsx (1)
215-215: No-op formatting changeTrailing newline/brace alignment only. Nothing to address.
archon-ui-main/test/config/api.test.ts (2)
35-44: LGTM: test asserts proxy-first behavior in “production” modeThe test now ensures VITE_API_URL is ignored when PROD is true, matching the new resolution order. Looks good.
238-238: No functional changeTrailing newline only.
archon-ui-main/src/config/api.ts (1)
61-65: Exports align; compute-on-import is fine with module resetsComputing API_FULL_URL/WS_URL at import time is acceptable since tests call vi.resetModules() to re-evaluate. No issues.
| const response = await fetch(`${credentialsService['baseUrl']}/api/health`, { | ||
| method: 'GET', | ||
| signal: controller.signal | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double-prefixing /api by using centralized API base path
If credentialsService.baseUrl already contains /api, this becomes /api/api/health. Prefer the centralized helper to construct the base path.
Apply:
@@
-import { credentialsService } from '../../services/credentialsService';
+import { credentialsService } from '../../services/credentialsService';
+import { getApiBasePath } from '../../config/api';
@@
- const response = await fetch(`${credentialsService['baseUrl']}/api/health`, {
+ const response = await fetch(`${getApiBasePath()}/health`, {
method: 'GET',
signal: controller.signal
});This keeps the health check correct regardless of whether you’re using a relative base or a full 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.
| const response = await fetch(`${credentialsService['baseUrl']}/api/health`, { | |
| method: 'GET', | |
| signal: controller.signal | |
| }); | |
| // archon-ui-main/src/components/layouts/MainLayout.tsx | |
| import React from 'react'; | |
| import { credentialsService } from '../../services/credentialsService'; | |
| import { getApiBasePath } from '../../config/api'; | |
| // … other code … | |
| const response = await fetch(`${getApiBasePath()}/health`, { | |
| method: 'GET', | |
| signal: controller.signal | |
| }); | |
| // … other code … |
🤖 Prompt for AI Agents
In archon-ui-main/src/components/layouts/MainLayout.tsx around lines 48 to 51,
the health check concatenates credentialsService.baseUrl with a hardcoded "/api"
which can produce "/api/api/health" when baseUrl already contains "/api"; change
the call to use the centralized API base-path helper (or derive a normalized
base without the "/api" prefix) and then append only "/health" so the final URL
is constructed consistently (e.g., use the app's API base helper to get baseUrl
and call `${apiBase}/health` or normalize credentialsService.baseUrl to strip
any trailing "/api" before appending "/health").
|
thanks @michaelphines this looks great, but we should probably add the example variable with a description in the .env.example as well |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.env.example (1)
39-42: Clarify boolean semantics and purpose of PROD to avoid confusion with Vite’s built-in PROD flagEnvironment variables are strings; the code treats only the literal "true" as enabled. Also, naming this flag PROD can be confused with Vite’s compile-time import.meta.env.PROD. Since this file is the primary source of truth for users, tighten the docs here to spell out accepted values and the exact behavior (Vite proxying /api/* and getApiUrl() returning ''). Consider the wording tweak below; rename is optional for a later PR.
Apply this doc-only diff to make behavior explicit:
-# When enabled, PROD mode will proxy ARCHON_SERVER_PORT through ARCHON_UI_PORT. This exposes both the -# Archon UI and API through a single port. This is useful when deploying Archon behind a reverse -# proxy where you want to expose the frontend on a single external domain. -PROD=false +# Enable UI -> API proxying +# Set to the string "true" (lowercase) to enable a Vite proxy that forwards /api/* from ARCHON_UI_PORT +# to ARCHON_SERVER_PORT. This exposes both the Archon UI and API on a single external port/domain +# (useful behind a reverse proxy). When PROD is "true", getApiUrl() returns an empty base URL and +# ignores VITE_API_URL; API calls should use /api/* paths. +# Any value other than "true" disables this behavior. +PROD=falseIf you want to further reduce ambiguity in a future PR (not required here), consider renaming to UI_PROXY_API or ENABLE_UI_PROXY and mapping it to import.meta.env.PROD in Vite config for backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.env.example(2 hunks)
🔇 Additional comments (1)
.env.example (1)
59-59: Minor comment formatting change is fineNo functional impact; aligns with surrounding comment style.
….1 (coleam00#467) Bumps [aquasecurity/trivy-action](https://github.com/aquasecurity/trivy-action) from 0.24.0 to 0.33.1. - [Release notes](https://github.com/aquasecurity/trivy-action/releases) - [Commits](aquasecurity/trivy-action@0.24.0...0.33.1) --- updated-dependencies: - dependency-name: aquasecurity/trivy-action dependency-version: 0.33.1 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…rds (#467) * fix: mock DB modules in github.test.ts to prevent phantom testuser/testrepo records Self-filtering tests called handleWebhook() with mocked signature verification but unmocked database modules. The code successfully wrote testuser/testrepo codebase and conversation records to the real SQLite DB before failing on the unmocked Octokit API call. The try/catch in the tests caught the Octokit error but the DB writes had already persisted. Added mock.module calls for @archon/core/db/conversations and @archon/core/db/codebases, and strengthened the three affected tests to assert that mockGetOrCreateConversation was called (proving the code progressed past self-filtering). * fix: also clear mockFindCodebaseByRepoUrl between self-filtering tests Prevents stale call counts from leaking across tests.
…rds (coleam00#467) * fix: mock DB modules in github.test.ts to prevent phantom testuser/testrepo records Self-filtering tests called handleWebhook() with mocked signature verification but unmocked database modules. The code successfully wrote testuser/testrepo codebase and conversation records to the real SQLite DB before failing on the unmocked Octokit API call. The try/catch in the tests caught the Octokit error but the DB writes had already persisted. Added mock.module calls for @archon/core/db/conversations and @archon/core/db/codebases, and strengthened the three affected tests to assert that mockGetOrCreateConversation was called (proving the code progressed past self-filtering). * fix: also clear mockFindCodebaseByRepoUrl between self-filtering tests Prevents stale call counts from leaking across tests.
…rds (coleam00#467) * fix: mock DB modules in github.test.ts to prevent phantom testuser/testrepo records Self-filtering tests called handleWebhook() with mocked signature verification but unmocked database modules. The code successfully wrote testuser/testrepo codebase and conversation records to the real SQLite DB before failing on the unmocked Octokit API call. The try/catch in the tests caught the Octokit error but the DB writes had already persisted. Added mock.module calls for @archon/core/db/conversations and @archon/core/db/codebases, and strengthened the three affected tests to assert that mockGetOrCreateConversation was called (proving the code progressed past self-filtering). * fix: also clear mockFindCodebaseByRepoUrl between self-filtering tests Prevents stale call counts from leaking across tests.
2 participants
Pull Request
Summary
There appears to be a
PRODenv variable that was intended to make a Vite proxy handle API request. It's nearly hooked up, but thePRODenvironment variable isn't forwarded from the .env file, and the health endpoint is not using the proxied version of the URL.Default behavior should remain unchanged, but it looks like in some cases the proxy was already used. This was true in places where
getApiUrlwas not used, for example: knowledgeBaseService usesAPI_BASE_URLand notAPI_FULL_URLChanges Made
/api/healthversion of the health endpoint rather than/healthPRODthrough docker-compose and Vite"true", make thePRODsetting override theVITE_API_URL(becauseVITE_API_URLalways seems to be set)HOSTtoallowedHostsType of Change
Affected Services
Testing
Test Evidence
Checklist
Breaking Changes
None
Additional Notes
Given that the proxy here is already partially used even with
PRODunset, I'm guessing it probably makes sense to make thePRODbehavior the default (or even only) behavior. I didn't think that decision was appropriately in scope for this PR, however.Summary by CodeRabbit