-
Notifications
You must be signed in to change notification settings - Fork 7
fix: pass port and https flag to WorkOS SDK when base URL requires it #140
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
Merged
+6
−3
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Validate
baseformat before usingnew URLto avoid opaque runtime failures.At Line 63,
new URL(base)assumesbaseis a fullhttp(s)URL. Because endpoint values are user-provided and currently unvalidated upstream, malformed or shorthand inputs can throw/misparse and later fail as genericfetch failed. Add protocol/hostname validation with a clear error message at this boundary.Suggested fix
📝 Committable suggestion
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.
it's a pre-existing issue of the code, it currently doesn't handle it either and just crashes with
ERR_INVALID_URLThere 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.
@Langleu, that's a fair point — the previousnew URL(base).hostnamecall had the exact same behaviour, so this PR doesn't regress anything there. I'll withdraw the concern for this PR.Would you like me to open a follow-up GitHub issue to track adding proper URL validation (with a user-friendly error message) as a future improvement? It'd be a clean, standalone change that won't block this fix.
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.
so it's already letting a user know that's not what is expected.
I think the better fix would be to adjust whatever is expected by the CLI in the first place vs just
Custom API endpoint.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.
🧩 Analysis chain
Good point — improving the prompt/placeholder/description at the input level is a cleaner UX fix. Let me find where that prompt lives in the codebase.
🏁 Script executed:
Length of output: 1284
🏁 Script executed:
Length of output: 8944
@Langleu, agreed — improving the CLI-level description is the right place for this. Looking at the code, there are two spots that could be made more explicit:src/bin.tsLine 369 — the--endpointflag description is just'Custom API endpoint', which doesn't hint at the expected format:src/utils/help-json.ts— the endpoint description there also appears to be a stale/truncated artifact ('n URL'), which would be worth fixing alongside.Interactive
env addflow insrc/commands/env.ts— the interactive wizard currently doesn't prompt forendpointat all (only name, type, and API key). If the target audience for this is local emulator users who runworkos env addinteractively, it may be worth adding an optional prompt there too, with placeholder text showing the expected format:Would you like me to open a follow-up issue to track the interactive-prompt gap and the
help-json.tsdescription fix?