Skip to content

Enable using base URLs with path extensions#1225

Open
Johennes wants to merge 1 commit into
livekit:mainfrom
Johennes:johannes/base-path
Open

Enable using base URLs with path extensions#1225
Johennes wants to merge 1 commit into
livekit:mainfrom
Johennes:johannes/base-path

Conversation

@Johennes

@Johennes Johennes commented Jul 7, 2026

Copy link
Copy Markdown

Before you submit your PR

Make sure the following is true before submitting your PR:

  • I have read the contributing guidelines and validated that this PR will be accepted.
  • I have read and followed the principles regarding breaking changes, testing, and code quality.

PR description

The SDK currently replaces the entire path of the base URL when forming request URLs. This makes it impossible to use base URLs that already contain path extensions. This change simply appends the computed path to the base path instead of overwriting it which matches what the Go SDK does.

Breaking changes

None.

MSRV

Not applicable.

Testing

Unit tests added.

Async

Not applicable.

@CLAassistant

CLAassistant commented Jul 7, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@Johennes Johennes force-pushed the johannes/base-path branch from 0e8c096 to dc527e2 Compare July 7, 2026 07:20
@Johennes Johennes marked this pull request as ready for review July 7, 2026 07:45
@Johennes Johennes requested a review from ladvoc as a code owner July 7, 2026 07:45
@Johennes Johennes force-pushed the johannes/base-path branch from dc527e2 to 883d33e Compare July 7, 2026 07:46
devin-ai-integration[bot]

This comment was marked as resolved.

@Johennes Johennes force-pushed the johannes/base-path branch from 883d33e to f36249a Compare July 7, 2026 18:44

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

Open in Devin Review

Comment thread livekit-api/src/services/mod.rs Outdated
Comment on lines +162 to +165
let url = url_with_path_suffix(
&current,
&format!("{}/{}.{}/{}", self.prefix, self.pkg, service, method),
);

@devin-ai-integration devin-ai-integration Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Failover region URLs will not inherit the original base path

When a request fails over to a region URL, current is replaced by the URL returned from failover::pick_next (livekit-api/src/services/twirp_client.rs:289-290), which comes from the /settings/regions discovery response. These region URLs are bare host URLs (e.g. https://region1.livekit.cloud) without any path prefix. So url_with_path_suffix(&current, ...) at livekit-api/src/services/twirp_client.rs:234-237 will only append the Twirp service path, without the original base URL's path extension. This is likely correct — region URLs point directly at LiveKit Cloud servers, not through the same reverse proxy — but it means the base-path feature only applies to the primary endpoint, not failover targets. Worth confirming this matches the intended behavior for users behind a path-based reverse proxy.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@Johennes Johennes Jul 7, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine because next comes from the server so it should hopefully already have any required path?

@Johennes Johennes force-pushed the johannes/base-path branch 3 times, most recently from be660d9 to 104ac83 Compare July 7, 2026 19:22
devin-ai-integration[bot]

This comment was marked as resolved.

@Johennes Johennes force-pushed the johannes/base-path branch from 104ac83 to c541f4b Compare July 7, 2026 19:35
@Johennes Johennes force-pushed the johannes/base-path branch from c541f4b to 55bf571 Compare July 7, 2026 19:53
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