Promisify OAuth service login#13892
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 51c3bad The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the SDK’s OAuth module to support a promise-based loginAsync() flow, reduces reliance on init(), and switches the authorization URL to be derived from the configured API basePath (rather than a hardcoded production OAuth URL).
Changes:
- Adds a
loginAsync()API that returns aPromise<LoginResult>and refactorslogin()to delegate to it. - Removes hardcoded OAuth environment/url constants and some previously exported types.
- Updates popup
postMessagehandling to accommodate redirects (origin check removed) and consolidates promise settlement via_settleLogin().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| packages/sdk/src/sdk/oauth/types.ts | Removes previously exported OAuth URL/env + unused schema/types; adds LoginResult. |
| packages/sdk/src/sdk/oauth/OAuth.ts | Implements promise-based login flow, moves message listener into constructor, updates auth URL construction and message handling. |
| .changeset/tricky-zoos-film.md | Declares a patch release for the OAuth changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
@rickyrombo I've opened a new pull request, #13893, to work on those changes. Once the pull request is ready, I'll request review from you. |
Contributor
|
@rickyrombo I've opened a new pull request, #13894, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
|
@rickyrombo I've opened a new pull request, #13895, to work on those changes. Once the pull request is ready, I'll request review from you. |
This was referenced Mar 11, 2026
Contributor
|
@rickyrombo I've opened a new pull request, #13896, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
🌐 Web preview readyPreview URL: https://audius-web-preview-pr-13892.audius.workers.dev Unique preview for this PR (deployed from this branch). |
`loginAsync()` stored resolve/reject in instance fields, so a second
concurrent call would overwrite those handlers, leaving the first
promise permanently unresolved.
## Changes
- **`OAuth.ts`**: Added an early-reject guard at the top of
`loginAsync()` — if `_currentLoginResolve` is already set (login
in-flight), the new call immediately rejects rather than clobbering the
existing handlers.
```ts
if (this._currentLoginResolve != null) {
return Promise.reject(new Error('A login is already in progress.'))
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
✨ Let Copilot coding agent [set things up for
you](https://github.com/AudiusProject/apps/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot)
— coding agent works faster and does higher quality work when set up for
your repo.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
When `window.open()` is blocked by the browser it returns `null`,
leaving the login promise permanently unresolved since no message event
or close-check can ever fire.
## Changes
- **Popup blocked detection**: Check the return value of `window.open()`
immediately after the call
- **Immediate rejection**: Reject the login promise with an actionable
error when the popup is blocked, instead of hanging indefinitely
- **Early return**: Skip setting up the close-check interval when
there's no window to monitor
```ts
this.activePopupWindow = window.open(fullOauthUrl, '', windowOptions)
if (!this.activePopupWindow) {
this._settleLogin(
new Error(
'The login popup was blocked. Please allow popups for this site and try again.'
)
)
return promise
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
`basePath` is optional in `OAuthConfig`, but the PKCE `{state, code}`
handler in `_receiveMessage` unconditionally used it when fetching
`/oauth/token` and `/oauth/me`, resulting in fetches to
`undefined/oauth/token` with no meaningful error surfaced to the caller.
## Changes
- **`packages/sdk/src/sdk/oauth/OAuth.ts`**: Added an early-exit guard
in the PKCE branch that rejects via `_settleLogin` with a descriptive
error if `basePath` is not configured, before any fetch is attempted.
```ts
if (!this.config.basePath) {
this._settleLogin(
new Error(
'basePath is required in SDK configuration for PKCE token exchange.'
)
)
return
}
```
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
Each `OAuth` instance attached a `window.addEventListener('message',
...)` in its constructor and never removed it, causing listener
accumulation across multiple SDK instantiations and risking duplicate
message processing.
## Changes
- **Removed constructor listener** — `window.addEventListener` no longer
called at construction time
- **Lazy registration in `loginAsync`** — listener is attached only when
entering the `postMessage` popup flow; guarded against
double-registration if `loginAsync` is called again while a login is in
progress
- **Cleanup in `_settleLogin`** — listener is removed (and popup check
interval cleared) as soon as the login resolves or rejects, via a stored
`_boundMessageHandler` reference
```ts
// Before: listener lived forever, one per OAuth instance
constructor(...) {
window.addEventListener('message', (e) => this._receiveMessage(e), false)
}
// After: listener scoped to each login attempt
async loginAsync({ redirectUri = 'postMessage', ... }) {
if (redirectUri === 'postMessage' && !this._boundMessageHandler) {
this._boundMessageHandler = (e) => this._receiveMessage(e)
window.addEventListener('message', this._boundMessageHandler, false)
}
// ...
}
private _settleLogin(resultOrError) {
// resolve/reject promise...
window.removeEventListener('message', this._boundMessageHandler, false)
this._boundMessageHandler = null
this._clearPopupCheckInterval()
}
```
- **Tests** — added `removeEventListener` + `crypto.getRandomValues` to
the window stub; new `OAuth message listener lifecycle` suite covering:
no listener on construction, listener attached on `loginAsync`, no
duplicate on re-entry, listener removed on settle
<!-- START COPILOT CODING AGENT TIPS -->
---
💡 You can make Copilot smarter by setting up custom instructions,
customizing its development environment and configuring Model Context
Protocol (MCP) servers. Learn more [Copilot coding agent
tips](https://gh.io/copilot-coding-agent-tips) in the docs.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: rickyrombo <3690498+rickyrombo@users.noreply.github.com>
raymondjacobson
approved these changes
Mar 11, 2026
…l methods out of internal group
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updates OAuth service
loginAsync()to be promise-based. Keepslogin()the same. Removes need forinit()to be called.Also removes some unused types. Updates the OAUTH_URL to be instead the API url from the config now that it redirects, which allows for handling multiple environments. However, since it redirects, removed the origin check on the postmessage since it won't come from the API server url.