Add Cancel to the SuperGrok / X Premium connect flow (APP-5638) - #15576
Draft
warp-agent-staging[bot] wants to merge 6 commits into
Draft
Add Cancel to the SuperGrok / X Premium connect flow (APP-5638)#15576warp-agent-staging[bot] wants to merge 6 commits into
warp-agent-staging[bot] wants to merge 6 commits into
Conversation
Closing the browser mid-connect left the button stuck on a disabled "Connecting" indicator: only a successful callback, a pasted code, or Disconnect (unavailable without tokens) cleared the in-flight state, so the user had to wait out the 5-minute callback timeout, and a retried Connect failed with "Another login may be in progress" while the old loopback listener still held the port. Retain the OauthAttempt's OauthCancellationHandle in the page's state alongside the manual-code fallback, and replace the disabled "Connecting" button with an enabled Cancel button. Cancel stops the loopback wait (releasing the bound port), clears the in-flight and manual-code-editor state, dismisses the connect toast, and returns to an idle, enabled Connect -- without reopening the browser or restarting the flow. SuperGrokSubscriptionConnectFinished now records a "cancelled" error code so every Initiated has a terminal Finished pair.
Contributor
Author
|
This PR was generated with Warp. Comment    |
…s auto-cancel - Track each SuperGrok connect attempt with a UUID (GrokOauthAttempt) so a stale completion (from an attempt Cancel already superseded) can no longer clobber a newer attempt's state -- both the loopback and manual completions now check the id before mutating anything. - Cancel no longer clears state or emits telemetry immediately: it only flips a cancelling flag and calls cancellation.cancel(). Teardown - clearing the attempt, the editor, the toast, and the one-time cancelled telemetry - happens in the loopback completion, since that future only resolves once the callback thread has actually dropped the listener. The row shows a disabled Cancelling button in between, so Connect is never exposed before the port is confirmed free. A duplicate Cancel click while already cancelling, or a stale/late completion, is a no-op. - member_byo_keys_allowed flipping false hides the whole connect row (and its only Cancel affordance); ApiKeysWidget now cancels any live attempt when that happens instead of stranding the listener. - Trimmed narrating test comments; removed the new oauth.rs timing test, since the structural drop-before-send guarantee it exercised is timing independent and already covered by the existing pre-cancellation test.
- Split OauthAttempt::finish() into an OauthReleaseSignal plus the result future. run_oauth_flow now signals release right after dropping the loopback listener, strictly before a raced-in callback's token exchange even starts, so a caller waiting only to retry isn't blocked on that network call. Updated the TUI call site (unaffected behavior: it still doesn't wait on anything before clearing its own state) and the two existing oauth tests for the new parameter, and added a deterministic, network-free regression test proving the release/result ordering. - GrokOauthAttempt now tracks `released` independently of `cancelling`. start_grok_oauth spawns two tasks off the split finish(): one marks `released` (or finalizes immediately if a Cancel is already pending) once the port is confirmed free, and one applies the eventual loopback result. cancel_grok_oauth finalizes immediately when already released instead of waiting on an in-flight exchange, via a new shared finalize_cancelled_grok_attempt helper. - submit_grok_code's manual-success branch now cancels the still-racing loopback attempt before clearing it, so it releases the port instead of holding it until CALLBACK_TIMEOUT. - DisconnectGrokSubscription now calls cancel_grok_oauth instead of clearing the attempt inline, so a live attempt (defensive-only case) follows the same cancel/release protocol and terminal telemetry. - Dropped a call-site comment duplicating the GrokOauthAttempt.id field doc, per the single-source comment rule.
warp-agent-staging
Bot
force-pushed
the
factory/grok-connect-cancel
branch
from
August 26, 2026 18:36
f8b68fc to
f8b0608
Compare
- Critical: the loopback result callback could arrive before the release-signal callback processed a pending Cancel, since sending on a channel does not serialize two independently-scheduled ctx.spawn completions. Unified Cancelled/Connected-pending-release into a single GrokOauthAttempt.outcome, and made both the release-signal task and the result task check it and finalize identically regardless of which one observes it first -- a result's own arrival already proves the listener was released, so it can finalize directly instead of assuming the release task ran first. - Addendum: manual-code success now defers publishing (storing tokens, enabling Disconnect) via the same outcome mechanism until release is confirmed, instead of taking the attempt immediately, so a user can no longer Disconnect and reconnect before the still-racing loopback listener has actually released port 56121. Applies immediately when the port happens to already be released by the time the code exchange resolves. - Narrowed the oauth_tests.rs release-ordering test's name and doc to what it actually proves deterministically (a caller can rebind after observing release without joining the full result), rather than a cross-thread ordering claim a bounded channel and independent thread scheduling can't guarantee without a production-only synchronization hook or a real (flaky, environment-dependent) network call.
finalize_cancelled_grok_attempt and finalize_connected_grok_attempt now document only their own current behavior, per AGENTS.md's rule against enumerating callers or implementation sequencing in doc comments.
cancelling_loopback_wait_releases_listener, release_signal_lets_a_caller_rebind_without_joining_the_result, and loopback_callback_is_delivered_after_the_listener_closes all scan the same PID-derived port range via bind_test_listener. Under normal parallel test execution, one test's just-released port could be claimed by another before its own exact-port rebind assertion ran, producing an intermittent AddrInUse failure. Serialize all three with a named serial_test lock so a regression suite for a port-release bug isn't itself flaky about ports.
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.
Description
If a user closes the browser (or otherwise abandons xAI's consent screen) mid-connect, the SuperGrok/X Premium "Connect" button in AI settings got stuck on a disabled "Connecting" indicator. Only a successful loopback callback, a pasted code, or Disconnect (unavailable without stored tokens) cleared the in-flight state, so the user had to wait out the 5-minute callback timeout, and a retried Connect failed with "Another login may be in progress" because the old loopback listener still held port 56121.
This retains the connect attempt's cancellation handle in the page's view state and replaces the disabled "Connecting" button with an enabled Cancel button.
OauthAttempt::finish()was split into anOauthReleaseSignal(resolves the instant the loopback listener is dropped) plus the token-exchange result future, so the GUI can react to "port is free" independently of a possibly slow token exchange that a raced-in browser callback may have already started.The attempt's eventual fate — cancelled, or connected via a pasted code that raced ahead of the loopback wait — is tracked as a single
GrokOauthAttempt.outcome. Two independent tasks (the release signal and the loopback result) both check and finalize that outcome, so finalization is correct regardless of which one happens to observe it first — no assumption is made about cross-task scheduling order. Concretely:member_byo_keys_allowedwhile an attempt is live,ApiKeysWidgetcancels it, since the row (and its only Cancel affordance) would otherwise disappear mid-flight.Telemetry decision:
SuperGrokSubscriptionConnectFinished.errornow also records"cancelled"(emitted exactly once, from the finalize step) so everySuperGrokSubscriptionConnectInitiatedstill gets a terminalFinishedpair.Out of scope (per discussion in the linked issue): letting Connect pre-empt an already in-flight attempt, and changing
CALLBACK_TIMEOUT.Linked Issue
Tracked in Linear: APP-5638 (no GitHub issue for this one).
Testing
Visual verification: a client was built from commit
9c0a8dce0and the flow was exercised on Linux (recording and screenshots in the managed artifacts block below). Observed: Connect switches to an enabled Cancel with the paste-code field and the connect toast; clicking Cancel returns the row to an enabled Connect with the field and toast gone; clicking Connect again opens a fresh browser window, confirming the retry the change exists for. The disabled "Cancelling…" transient wasn't caught on screen, consistent with it normally resolving in well under a second. The two commits after9c0a8dce0(doc-comment wording, aserial_testattribute on the port-binding tests) don't change any runtime behavior, so this capture still reflects the current head.app/src/settings_view/warp_agent_page_tests.rs: pure state-transition coverage forgrok_subscription_button_action— idle → in-flight/Cancel → cancelling/Cancelling → idle, and stored-tokens-take-precedence over any attempt phase.crates/ai/src/grok_subscription/oauth_tests.rs::release_signal_lets_a_caller_rebind_without_joining_the_result: proves a caller can rebind the port after observing the release signal without joining the full result. Note on scope: this deliberately does not assert a cross-thread ordering guarantee between the release-signal consumer and the result consumer, because that ordering can't be pinned deterministically without either a production-only synchronization hook or a real (slow, environment-dependent) network call — and the GUI no longer depends on that ordering at all, since both consumers now independently check and correctly finalize the sharedoutcomeregardless of which runs first.cargo check -p warp --lib,cargo check -p warp_tui,cargo clippy -p warp --lib -- -D warnings,cargo clippy -p warp_tui --lib -- -D warnings,cargo clippy -p ai --lib --tests -- -D warnings, andcargo fmt --check(warp, warp_tui, ai) all pass.cargo test -p warp --lib settings_view::warp_agent_page::tests(8/8 pass) andcargo test -p ai grok_subscription::oauth(7/7 pass, confirmed stable under normal parallel execution after serializing the port-binding tests).Not run:
cargo test -p warp_tui. Its test target fails to compile in this environment due to a pre-existing, unrelatedE0046(missing get_cursor_shape) error incrates/warpui's platform delegates — reproduced identically with this PR's changes fully reverted, so it isn't caused by this change.cargo check -p warp_tui(without--tests) passes, confirming the TUI call-site update compiles correctly.I have manually tested my changes locally with
./script/run9c0a8dce0via computer use (see the managed artifacts block below for the recording and screenshots).Agent Mode
Computer-use video recordings
Testing xAI Connect/Cancel flow in Warp Agent settings: Recording the settings row for 'X Premium or SuperGrok subscription': clicking Connect to open the xAI auth browser, closing it without authenticating, observing the in-flight/Cancel state, clicking Cancel and observing the transition back to Connect, then clicking Connect again to verify a fresh attempt starts.
Computer-use screenshots (8)
Initial state: X Premium/SuperGrok subscription row showing an enabled "Connect" button
Right after clicking Connect: a toast appears reading 'Opening your browser to connect your SuperGrok subscription... Copy URL', the row's button changed to 'Cancel', and a 'Paste sign-in code' field appeared
Step 1 - Initial state: 'Use your X Premium or SuperGrok subscription' row with enabled 'Connect' button
Step 2a - Right after clicking Connect: toast reads 'Opening your browser to connect your SuperGrok subscription... Copy URL', row button changed to 'Cancel', and a 'Paste sign-in code' field appeared
Step 2b - Chromium browser opened to xAI's real OAuth sign-in page ('Log into your account') with login options for Google, X, Apple, or email
Step 3 - After closing the browser without authenticating, back in Warp settings: the toast 'Opening your browser to connect your SuperGrok subscription... Copy URL' is still visible, row shows 'Cancel' button and 'Paste sign-in code' input field (in-flight/pending state)
Step 4 - After clicking Cancel and settling: the row reverted to an enabled 'Connect' button, no intermediate disabled/'Cancelling' state was visible in the screenshot taken immediately after the click
Step 5 - Clicking Connect again successfully opened a fresh Chromium window with the xAI 'Log into your account' sign-in page, confirming a new auth attempt started