fix: avoid duplicate region prefix in getApiEndpoint for regional API URLs#2941
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 |
Greptile SummaryThis PR adds a guard in Key changes:
Concerns:
Confidence Score: 4/5Safe for the primary same-region duplicate-prefix scenario, but the cross-region edge case with a pre-regionalized endpoint is unaddressed and there are no tests. The fix correctly resolves the described same-region double-prefix bug. However, it is incomplete for the cross-region mismatch case, and the complete lack of PR description or test plan makes it hard to verify the full scope of the intended fix. src/lib/stores/sdk.ts — the only changed file; the new guard needs to handle cross-region mismatches and ideally be covered by tests. Important Files Changed
Reviews (1): Last reviewed commit: "avoid duplicate region prefix in getApiE..." | Re-trigger Greptile |
| let subdomain = isMultiRegionSupported(url) ? getSubdomain(region) : ''; | ||
| if (subdomain && hostname.startsWith(subdomain)) { | ||
| subdomain = ''; | ||
| } | ||
|
|
||
| return `${protocol}//${subdomain}${hostname}/v1`; |
There was a problem hiding this comment.
No tests added for the new guard
The getApiEndpoint function is called in many places across the codebase, yet there are no unit tests for this particular fix. A test covering at least the two key scenarios would make the intention clear and prevent regressions:
APPWRITE_ENDPOINT = "https://fra.cloud.appwrite.io"+getApiEndpoint('fra')→ should returnhttps://fra.cloud.appwrite.io/v1(no duplicate prefix)APPWRITE_ENDPOINT = "https://cloud.appwrite.io"+getApiEndpoint('fra')→ should returnhttps://fra.cloud.appwrite.io/v1(prefix added normally)
Without tests, it is difficult to verify the fix is correct and to protect against future regressions.
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