Conversation
Because: - open() resolved on a failed connect, so openChannel()'s catch never ran and never cleared its client reference; every later call then returned early on "Pairing channel already open!". - The supplicant could not retry a channel after a flaky network or a consumed channel id, short of a full page reload. - create() rethrows and attaches its listeners before `connected`; open() did neither. This commit: - Rejects from open() once the error event has gone out. - Guards openChannel()'s catch with isPostCompletionReconnect() so the post-OAuth reload still reaches the success screen. - Attaches the channel listeners before dispatching `connected`. - Covers the retry path, the listener order, and the reload guard. Closes #FXA-14483
dschom
commented
Sep 4, 2026
| // A consumed channel refusing the socket is the post-OAuth reload, not a | ||
| // failure. open() dispatches an `error` event before it rejects, so | ||
| // handleChannelError has usually settled the state by now; fail() is a | ||
| // no-op once it has, and still catches the errors raised before any |
dschom
commented
Sep 4, 2026
| // Send OAuth request to authority | ||
| if (!this._channel) { | ||
| throw new Error('Channel no longe exists!') | ||
| throw new Error('Channel no longe exists!'); |
Contributor
Author
There was a problem hiding this comment.
Typo., should be 'Channel no longer exists!'
dschom
marked this pull request as ready for review
September 4, 2026 23:58
Because: - The guard comment's "no-op once it has" had no clear antecedent. - The channel-missing error read "no longe exists". This commit: - Rewrites the openChannel guard comment to say why fail() still runs for the errors that reject before any event is dispatched. - Corrects the thrown message to "Channel no longer exists!".
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
1 participant
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.
Because
open()resolved on a failed connect, soopenChannel()'s catch never cleared its client reference; every later call returned early on "Pairing channel already open!".create()rethrows and attaches its listeners beforeconnected;open()did neither.This pull request
open()once the error event has gone out.openChannel()'s catch withisPostCompletionReconnect().connected.Issue that this pull request solves
Closes: FXA-14483
Checklist
Put an
xin the boxes that applyHow to review (Optional)
open()catch inpairing-channel.ts, and theopenChannel()catch inpairing-supplicant-integration.ts.pairing-channel.ts, then the integration guard that absorbs it, then the tests.open()rejecting, the FXA-13616 post-OAuth Android reload would otherwise navigate to the failure screen. Removing the guard failsopen failure while still Connecting is ignored when completion marker is setwithExpected "connecting", Received "failed".Screenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
fxa-14444; retarget tomainonce that merges.pairing-supplicant-integration.tsare prettier churn from the pre-commit hook; the actual change there is the 7-line guard.awaitseparates the dispatch from the registration. Included becausecreate()treats the order as load-bearing.TLS Alert: DECODE_ERROR) is another connect rejection reaching this catch block; expect a textual conflict if it lands first.