fix: normalize regional API base URL and prevent invalid multi-region…#2942
fix: normalize regional API base URL and prevent invalid multi-region…#2942HarshMN2345 wants to merge 1 commit intomainfrom
Conversation
WalkthroughA new utility module Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (1)
src/lib/helpers/apiEndpoint.ts (1)
71-85: Add focused tests for endpoint normalization edge cases.Please add helper-level tests for: repeated prefixes, hostnames with ports, unknown region values, and
isMultiRegion=falsepassthrough behavior to lock this logic down.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/helpers/apiEndpoint.ts` around lines 71 - 85, Add focused unit tests that exercise buildRegionalV1Endpoint and its helpers (stripLeadingRegionSubdomain, getRegionSubdomain): 1) verify repeated region prefixes are normalized (e.g., "us-us.example.com" -> single "us."), 2) ensure hostnames with ports (e.g., "example.com:3000") preserve the port and produce correct "/v1" path, 3) cover unknown/undefined region values to confirm getRegionSubdomain returns the expected fallback and the endpoint remains valid, and 4) assert that when isMultiRegion=false the function returns the hostname unchanged (passthrough) with protocol and "/v1". Include assertions for both protocol variants and edge-case inputs to lock down normalization behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/helpers/apiEndpoint.ts`:
- Around line 71-85: Add focused unit tests that exercise
buildRegionalV1Endpoint and its helpers (stripLeadingRegionSubdomain,
getRegionSubdomain): 1) verify repeated region prefixes are normalized (e.g.,
"us-us.example.com" -> single "us."), 2) ensure hostnames with ports (e.g.,
"example.com:3000") preserve the port and produce correct "/v1" path, 3) cover
unknown/undefined region values to confirm getRegionSubdomain returns the
expected fallback and the endpoint remains valid, and 4) assert that when
isMultiRegion=false the function returns the hostname unchanged (passthrough)
with protocol and "/v1". Include assertions for both protocol variants and
edge-case inputs to lock down normalization behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dae0f278-bde9-41db-89eb-598cb928dce8
📒 Files selected for processing (2)
src/lib/helpers/apiEndpoint.tssrc/lib/stores/sdk.ts
Greptile SummaryThis PR refactors the regional API endpoint logic out of Key changes:
Issues found:
Confidence Score: 4/5The PR fixes a real cross-region bug but introduces a behavioral regression for the no-region case on multi-region instances — should be addressed before merging. One P1 finding: when src/lib/helpers/apiEndpoint.ts — the no-region guard in Important Files Changed
Reviews (1): Last reviewed commit: "fix: normalize regional API base URL and..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (!isMultiRegion) { | ||
| return `${protocol}//${hostname}/v1`; | ||
| } | ||
|
|
||
| const hostWithoutRegion = stripLeadingRegionSubdomain(hostname); | ||
| const subdomain = getRegionSubdomain(region); | ||
|
|
||
| return `${protocol}//${subdomain}${hostWithoutRegion}/v1`; |
There was a problem hiding this comment.
When buildRegionalV1Endpoint is called with isMultiRegion = true but region = undefined (which happens for the initial const endpoint = getApiEndpoint() at sdk.ts:67), the function will:
- Strip any existing region prefix from
hostnameviastripLeadingRegionSubdomain - Prepend
getRegionSubdomain(undefined)→''
This produces a hostname without any region prefix.
The original code avoided this problem because it only mutated the hostname when subdomain was non-empty (if (subdomain && hostname.startsWith(subdomain)) ...). With getSubdomain(undefined) returning '', the original hostname passed through unchanged.
Affected scenario: Any self-hosted instance with PUBLIC_APPWRITE_MULTI_REGION=true where APPWRITE_ENDPOINT (or the browser URL) contains a region prefix (e.g. fra.example.com). Calling getApiEndpoint() with the new code would strip fra. and route the console clients to example.com/v1 instead of the intended fra.example.com/v1.
A minimal guard fixes this:
if (!isMultiRegion) {
return `${protocol}//${hostname}/v1`;
}
const subdomain = getRegionSubdomain(region);
if (!subdomain) {
// No specific region requested — return the hostname as-is without stripping.
return `${protocol}//${hostname}/v1`;
}
const hostWithoutRegion = stripLeadingRegionSubdomain(hostname);
return `${protocol}//${subdomain}${hostWithoutRegion}/v1`;| import { | ||
| REGION_FRA, | ||
| REGION_NYC, | ||
| REGION_SFO, | ||
| REGION_SGP, | ||
| REGION_SYD, | ||
| REGION_TOR, | ||
| SUBDOMAIN_FRA, | ||
| SUBDOMAIN_NYC, | ||
| SUBDOMAIN_SFO, | ||
| SUBDOMAIN_SGP, | ||
| SUBDOMAIN_SYD, | ||
| SUBDOMAIN_TOR | ||
| } from '$lib/constants'; | ||
|
|
||
| /** Ordered list of region DNS prefixes (e.g. `fra.`) for stripping from API hostnames. */ | ||
| const REGION_SUBDOMAIN_PREFIXES: readonly string[] = [ | ||
| SUBDOMAIN_FRA, | ||
| SUBDOMAIN_NYC, | ||
| SUBDOMAIN_SYD, | ||
| SUBDOMAIN_SFO, | ||
| SUBDOMAIN_SGP, | ||
| SUBDOMAIN_TOR | ||
| ]; | ||
|
|
||
| /** | ||
| * Removes leading Appwrite Cloud region label(s) from `hostname` (e.g. `fra.`). | ||
| * Strips repeatedly so a doubled prefix (`fra.fra.cloud...`) does not become | ||
| * `nyc.fra.cloud...` after prepending another region. | ||
| */ | ||
| export function stripLeadingRegionSubdomain(hostname: string): string { | ||
| let host = hostname; | ||
| let changed = true; | ||
| while (changed) { | ||
| changed = false; | ||
| for (const prefix of REGION_SUBDOMAIN_PREFIXES) { | ||
| if (host.startsWith(prefix)) { | ||
| host = host.slice(prefix.length); | ||
| changed = true; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| return host; | ||
| } | ||
|
|
||
| /** Region prefix (e.g. `fra.`) used before the API hostname when multi-region is enabled. */ | ||
| export function getRegionSubdomain(region?: string): string { | ||
| switch (region) { | ||
| case REGION_FRA: | ||
| return SUBDOMAIN_FRA; | ||
| case REGION_SYD: | ||
| return SUBDOMAIN_SYD; | ||
| case REGION_NYC: | ||
| return SUBDOMAIN_NYC; | ||
| case REGION_SFO: | ||
| return SUBDOMAIN_SFO; | ||
| case REGION_SGP: | ||
| return SUBDOMAIN_SGP; | ||
| case REGION_TOR: | ||
| return SUBDOMAIN_TOR; | ||
| default: | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Builds the `/v1` API base URL (protocol + host + `/v1`). | ||
| * When `isMultiRegion` is true, strips any known region prefix from the host, then prepends the requested region. | ||
| */ | ||
| export function buildRegionalV1Endpoint( | ||
| protocol: string, | ||
| hostname: string, | ||
| region: string | undefined, | ||
| isMultiRegion: boolean | ||
| ): string { | ||
| if (!isMultiRegion) { | ||
| return `${protocol}//${hostname}/v1`; | ||
| } | ||
|
|
||
| const hostWithoutRegion = stripLeadingRegionSubdomain(hostname); | ||
| const subdomain = getRegionSubdomain(region); | ||
|
|
||
| return `${protocol}//${subdomain}${hostWithoutRegion}/v1`; | ||
| } |
There was a problem hiding this comment.
The project convention (documented in AGENTS.md and reflected by every other helper) is to co-locate a *.test.ts file alongside each helper module. For example: array.test.ts, date.test.ts, string.test.ts, etc. live right next to their corresponding helpers.
apiEndpoint.ts introduces meaningful URL-manipulation logic (stripLeadingRegionSubdomain, getRegionSubdomain, buildRegionalV1Endpoint) but ships with no test file, making it harder to catch regressions.
Consider adding src/lib/helpers/apiEndpoint.test.ts with cases such as:
- Multi-region disabled → hostname unchanged
- Switching region when hostname already has a different prefix (
fra.cloud.appwrite.io→ regionnyc→nyc.cloud.appwrite.io/v1) - Doubling the same prefix (
fra.fra.cloud.appwrite.io→fra.cloud.appwrite.io/v1) - No region requested (
undefined) on a multi-region URL → hostname unchanged
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
… hosts
What does this PR do?
(Provide a description of what this PR does.)
Test Plan
(Write your test plan here. If you changed any code, please provide us with clear instructions on how you verified your changes work.)
Related PRs and Issues
(If this PR is related to any other PR or resolves any issue or related to any issue link all related PR and issues here.)
Have you read the Contributing Guidelines on issues?
(Write your answer here.)
Summary by CodeRabbit