Skip to content

Fix getDefaultServerBaseUrl#17

Merged
achimala merged 1 commit into
achimala:mainfrom
dhvcc:fix/server-port
Mar 2, 2026
Merged

Fix getDefaultServerBaseUrl#17
achimala merged 1 commit into
achimala:mainfrom
dhvcc:fix/server-port

Conversation

@dhvcc
Copy link
Copy Markdown
Contributor

@dhvcc dhvcc commented Mar 2, 2026

I think there is a breaking change for some setups introduced in 95379bd

Since this commit, browser defaults to 4311 on non-local ports. If you just run a simple port tunnel via Cloudflare or whatever - this will result in your hosted app trying to connect to yourdomain.com:4311 for api calls, which is not going to work

I'm not sure how important are those lines for the working of http://farfield.app, but I'm just going to leave it there for now as it's the only way I was able to continue using the app remotely

Summary by CodeRabbit

  • Bug Fixes
    • Improved server base URL detection to use the browser's origin instead of manual construction, ensuring more reliable connection handling in production environments while maintaining existing localhost behavior.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Mar 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5428b2 and ba73662.

📒 Files selected for processing (1)
  • apps/web/src/lib/server-target.ts

📝 Walkthrough

Walkthrough

Simplified the getDefaultServerBaseUrl function by replacing manual protocol/host/port string construction with window.location.origin for non-localhost environments. Localhost path handling logic unchanged.

Changes

Cohort / File(s) Summary
Server Base URL Simplification
apps/web/src/lib/server-target.ts
Replaced manual protocol and origin construction with window.location.origin for non-localhost cases; localhost behavior remains unchanged.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A simpler path we now do take,
No need to build what's baked in place,
The origin whispers, smooth and clean,
Where window.location leads the way,
Our localhost rests in peace today! 🌟

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Fix getDefaultServerBaseUrl' directly and specifically refers to the function being fixed, aligning with the main change in the PR which modifies how this function constructs server base URLs.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@achimala
Copy link
Copy Markdown
Owner

achimala commented Mar 2, 2026

Hm, this shouldn't be needed for farfield.app either. I'm not sure how this slipped in, sorry for missing it

@achimala achimala merged commit ea125d0 into achimala:main Mar 2, 2026
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants