-
-
Notifications
You must be signed in to change notification settings - Fork 83
fix: supertokens URI for ip vs domain #489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds a private helper to compute SUPERTOKENS_CONNECTION_URI with IP-vs-domain and protocol handling, updates its usage in the install flow, adjusts app domain string handling in front-end config, and bumps the CLI package version from 0.1.9 to 0.1.10. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Install
participant Env as _update_environment_variables
participant Helper as _get_supertokens_connection_uri
participant IP as ipaddress
User->>Install: run install
Install->>Env: update environment variables
Env->>Helper: compute SUPERTOKENS_CONNECTION_URI(protocol, api_host, port)
Helper->>IP: attempt parse api_host as IP
alt api_host is IP
Helper-->>Env: return http(s)://<ip>:<port>
else api_host is domain
Helper-->>Env: return http(s)://<host>:<port>
end
Env-->>Install: env vars updated
Install-->>User: install complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cli/app/commands/install/run.py(3 hunks)cli/pyproject.toml(1 hunks)
🔇 Additional comments (2)
cli/pyproject.toml (1)
3-3: LGTM! Version bump aligns with the fix.The version increment from 0.1.9 to 0.1.10 is appropriate for the SuperTokens URI fix in this PR.
cli/app/commands/install/run.py (1)
473-474: Clarify and refine SUPERTOKENS_CONNECTION_URI TODO
Update the TODO to state that this HTTP-only workaround should be removed once TLS is supported for SuperTokens. Verify that_get_supertokens_connection_uriproperly distinguishes IP (no port) vs domain (with port) and that all three invocations of_update_environment_variablesinrun.pyapply it consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
view/app/config/appInfo.ts (1)
5-6: Use URL API for robust URL manipulation.
The TEMP-masking can collide ifbaseUrlcontains"TEMP"(e.g.http://temp-server.com/api) and masking://isn’t needed.
Refactor:- const apiDomain = baseUrl.replace('://','TEMP').replace('/api','').replace('TEMP','://'); + const url = new URL(baseUrl); + url.pathname = url.pathname.replace(/\/api$/,''); + const apiDomain = url.toString().replace(/\/$/,'');Or, if you only need to drop a trailing
/api:- const apiDomain = baseUrl.replace('://','TEMP').replace('/api','').replace('TEMP','://'); + const apiDomain = baseUrl.replace(/\/api$/,'');Please clarify which IP vs domain edge case this change targets.
Issue
Link to related issue(s):
Description
Short summary of what this PR changes or introduces.
Scope of Change
Select all applicable areas impacted by this PR:
Screenshot / Video / GIF (if applicable)
Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.
Related PRs (if any)
Link any related or dependent PRs across repos.
Additional Notes for Reviewers (optional)
Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).
Developer Checklist
To be completed by the developer who raised the PR.
Reviewer Checklist
To be completed by the reviewer before merge.
Summary by CodeRabbit
Bug Fixes
Chores