fix(web): align no-.env API proxy fallback with server default#1159
fix(web): align no-.env API proxy fallback with server default#1159LaplaceYoung wants to merge 1 commit intocoleam00:devfrom
Conversation
📝 WalkthroughWalkthroughA new utility module resolves the API port with a default of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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 (2)
packages/web/src/lib/api-port.ts (1)
1-1: UseDEFAULT_API_PORTas the single fallback source across web runtime code too.There is still a hardcoded
'3090'fallback inpackages/web/src/lib/api.ts:15-21. ImportingDEFAULT_API_PORTthere would prevent future drift between Vite-time and runtime defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/api-port.ts` at line 1, The codebase has a hardcoded '3090' fallback in the runtime API URL code; import and use the exported DEFAULT_API_PORT constant instead to keep a single source of truth. Modify the module that constructs the runtime API port (the code currently falling back to '3090') to import DEFAULT_API_PORT from the export (DEFAULT_API_PORT) and replace the literal '3090' with that constant, ensuring any type conversion (string vs number) remains correct when building the URL.packages/web/src/lib/api-port.test.ts (1)
14-16: Add edge-case tests for trimming and invalidPORTvalues.Please include a case for
' 3090 'and (if validation is added) explicit throw assertions for invalid inputs like'abc'and'70000'to lock behavior.Proposed test additions
test('uses PORT from env when provided', () => { expect(resolveApiPort('3090')).toBe('3090'); + expect(resolveApiPort(' 3090 ')).toBe('3090'); }); + + test('throws when PORT is invalid', () => { + expect(() => resolveApiPort('abc')).toThrow(); + expect(() => resolveApiPort('70000')).toThrow(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/web/src/lib/api-port.test.ts` around lines 14 - 16, Add edge-case tests for resolveApiPort: include a test asserting resolveApiPort(' 3090 ') returns '3090' (trimming whitespace), and add tests for invalid inputs such as 'abc' and '70000' that assert the expected failure behavior (if you’ve implemented validation, assert that resolveApiPort throws for those inputs; otherwise assert the documented fallback behavior). Use descriptive test names like "trims whitespace from PORT" and "throws on invalid PORT values" to make intent clear and target the resolveApiPort function in the existing test file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/web/src/lib/api-port.ts`:
- Around line 7-9: The resolveApiPort function currently returns any non-empty
string; modify it to validate and fail fast: trim the input (resolveApiPort), if
empty return DEFAULT_API_PORT; otherwise ensure the value matches /^\d+$/ then
parseInt(value,10) and assert the parsed port is between 1 and 65535; if
validation passes return the numeric port as a string, otherwise throw a clear
Error that includes the invalid port value (e.g., "Invalid API port: '<value>' —
must be an integer between 1 and 65535"), referencing resolveApiPort and
DEFAULT_API_PORT so callers can find and update behavior.
---
Nitpick comments:
In `@packages/web/src/lib/api-port.test.ts`:
- Around line 14-16: Add edge-case tests for resolveApiPort: include a test
asserting resolveApiPort(' 3090 ') returns '3090' (trimming whitespace), and add
tests for invalid inputs such as 'abc' and '70000' that assert the expected
failure behavior (if you’ve implemented validation, assert that resolveApiPort
throws for those inputs; otherwise assert the documented fallback behavior). Use
descriptive test names like "trims whitespace from PORT" and "throws on invalid
PORT values" to make intent clear and target the resolveApiPort function in the
existing test file.
In `@packages/web/src/lib/api-port.ts`:
- Line 1: The codebase has a hardcoded '3090' fallback in the runtime API URL
code; import and use the exported DEFAULT_API_PORT constant instead to keep a
single source of truth. Modify the module that constructs the runtime API port
(the code currently falling back to '3090') to import DEFAULT_API_PORT from the
export (DEFAULT_API_PORT) and replace the literal '3090' with that constant,
ensuring any type conversion (string vs number) remains correct when building
the URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1924d158-5aa6-434c-ae43-d1156468a6c2
📒 Files selected for processing (3)
packages/web/src/lib/api-port.test.tspackages/web/src/lib/api-port.tspackages/web/vite.config.ts
| export function resolveApiPort(port: string | undefined): string { | ||
| const value = port?.trim(); | ||
| return value ? value : DEFAULT_API_PORT; |
There was a problem hiding this comment.
Validate PORT format/range and fail fast on invalid values.
resolveApiPort currently accepts any non-empty string. Values like abc or 70000 should throw a clear error instead of producing a broken proxy target at runtime.
Proposed fix
export function resolveApiPort(port: string | undefined): string {
const value = port?.trim();
- return value ? value : DEFAULT_API_PORT;
+ if (!value) return DEFAULT_API_PORT;
+
+ const parsed = Number(value);
+ if (!Number.isInteger(parsed) || parsed < 1 || parsed > 65535) {
+ throw new Error(`Invalid PORT value "${value}". Expected integer 1-65535.`);
+ }
+
+ return value;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/web/src/lib/api-port.ts` around lines 7 - 9, The resolveApiPort
function currently returns any non-empty string; modify it to validate and fail
fast: trim the input (resolveApiPort), if empty return DEFAULT_API_PORT;
otherwise ensure the value matches /^\d+$/ then parseInt(value,10) and assert
the parsed port is between 1 and 65535; if validation passes return the numeric
port as a string, otherwise throw a clear Error that includes the invalid port
value (e.g., "Invalid API port: '<value>' — must be an integer between 1 and
65535"), referencing resolveApiPort and DEFAULT_API_PORT so callers can find and
update behavior.
|
Hi @LaplaceYoung — thanks for working on this. Before this merges, I want to flag that I think this PR is based on the (reversed) RCA in issue #1152 and would actually introduce the bug the issue describes. Sharing verification so we can align. The server does not default to 3000
// line 33 (stale JSDoc — this is probably what misled the issue):
* - Otherwise: use default 3000
...
// line 49 (actual code):
const basePort = 3090;The JSDoc on line 33 is wrong; the real fallback is I verified with a direct unit invocation (no PORT env var, no And Vite's Both sides already default to 3090. Changing Vite's fallback to 3000 would create a real mismatch on fresh clones with no The actual mismatch sourceI wrote up the full analysis on the issue: #1152 (comment) TL;DR — the mismatch isn't in code defaults; it comes from:
Alternative fixI opened a PR taking the other direction — align Let me know what makes sense to you and the maintainers. |
|
Thank you for the detailed analysis! You're absolutely right. I'm honored that my PR could provide the inspiration for your deep dive into the root cause. I agree that aligning everything to the actual 3090 default, as you proposed in your PR, is likely the better direction. Let's see what the maintainers think, but I'm happy to follow your lead on this |
|
Thanks for looking at this, @LaplaceYoung — closing because the underlying issue is already resolved and this PR moves in the wrong direction. What's actually on `dev`
Server and Vite already match at 3090 when no `.env` exists. This PR would change the Vite fallback to 3000, introducing the very mismatch the original issue complained about (just in the opposite direction). Why #1152's RCA was misleadingThe original bug report asserted the server defaulted to 3000 and Vite to 3090. That was factually reversed — the server moved 3000 → 3090 in #318 (Hono migration) but `.env.example` and the setup wizard still documented/forced 3000. The real break case was:
@leex279 walked through this in the issue's closing comment: #1152 The actual fix already landed#1271 (merged 2026-04-17) aligned everything on 3090:
Single source of truth is now `port-allocation.ts:49`. Salvageable partsThe `api-port.ts` centralization idea (trim blanks → fallback) is fine in principle, but without a port mismatch to fix there's no motivating behavior change — and the tests would lock in the wrong default. If there's a future need for a shared port helper, a separate PR using 3090 as the default would work. Thanks again for digging in; this one just got fixed through a different angle before your PR surfaced. |
3 participants
Summary
@archon/server(3000), so fresh installs without.envno longer proxy to:3090packages/web/src/lib/api-port.tsWhy
Issue #1152 reports a no-
.envinstall path where server binds:3000but Vite proxies/apito:3090, causingECONNREFUSEDand a broken web UI.Testing
bun test packages/web/src/lib/api-port.test.tsbun test packages/web/src/lib/Closes #1152
Summary by CodeRabbit
Tests
Chores