Don't flash sign-in modal at signed-in users during Agents window startup - #329269
Merged
Tyler James Leonhardt (TylerLeonhardt) merged 2 commits intoAug 5, 2026
Merged
Conversation
Copilot started reviewing on behalf of
Tyler James Leonhardt (TylerLeonhardt)
August 5, 2026 21:13
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents transient sign-in UI while the Agents window’s GitHub account state is unresolved.
Changes:
- Adds a shared conditional-auth state helper.
- Gates the setup modal and configuration notification until authentication resolves.
- Adds a truth-table unit test.
Show a summary per file
| File | Description |
|---|---|
src/vs/sessions/browser/sessionsAuthGate.ts |
Defines resolved authentication states. |
src/vs/sessions/browser/sessionsSetUpService.ts |
Gates modal reactions during startup. |
src/vs/sessions/contrib/providers/agentHost/browser/agentHostDiscoveredConfigNotification.ts |
Suppresses unresolved-state notifications. |
src/vs/sessions/test/browser/sessionsAuthGate.test.ts |
Tests authentication-state classification. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
Tyler James Leonhardt (TylerLeonhardt)
marked this pull request as ready for review
August 5, 2026 21:27
roblourens
approved these changes
Aug 5, 2026
The conditional-auth UI added in #328990 conflated "auth not resolved yet" with "signed out". IDefaultAccountService.currentDefaultAccount is a synchronous getter that returns null for everyone until the first async resolution completes — and that initial resolution fires no change event. During the startup gap a signed-in user therefore reads as signed-out, so the sign-in modal and the discovered-config nudge flash. Worse, once auth finally resolves nothing retires the already-raised modal, so it stays up. Gate both reactive consumers (sessionsSetUpService and the discovered config notification) on a resolved-auth signal, learned via a one-shot getDefaultAccount() await — the only reliable indicator that resolution happened, since the initial null->account assignment is event-silent. While unresolved, neither consumer acts, so gap-time Claude native<->proxy churn is inert and the normal sign-in watch owns the signed-in path. Consolidate the shared unresolved-vs-signed-out logic both consumers were duplicating into a unit-tested conditionalAuthState helper. The intended signed-out conditional-auth behaviour from #328990 is preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses review feedback: gating _onUsableWithoutGitHubChanged on _accountResolved dropped any usability transition that landed during the unresolved window without replaying it. The native paths re-read the current usability after they await the account (via _showWelcome -> _mustForceGitHubSignIn), but the web path (_checkWebAuth) has no such post-resolution re-check, so a genuinely signed-out, opted-in user whose agent became usable during the gap would stay stranded on the sign-in dialog that nothing else retires. When the account resolves, replay the current usability state — scoped to usable === true, the only transition that was wrongly dropped; a not-usable state is still owned by the initial setup flow. Signed-in users remain a no-op (the handler early-returns), so the original fix is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tyler James Leonhardt (TylerLeonhardt)
force-pushed
the
tyleonha/signed-in-startup-signin-modal
branch
from
August 5, 2026 22:45
9d9aae3 to
5c9a7d4
Compare
Tyler James Leonhardt (TylerLeonhardt)
enabled auto-merge (squash)
August 5, 2026 22:46
Dmitriy Vasyura (dmitrivMS)
approved these changes
Aug 5, 2026
Tyler James Leonhardt (TylerLeonhardt)
deleted the
tyleonha/signed-in-startup-signin-modal
branch
August 5, 2026 23:34
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.
The bug
With
chat.agentHost.allowSignedOutWhenUsable: trueand already signed in to GitHub, launching the Agents window produced a broken sequence:Root cause
The conditional-auth UI added in #328990 conflates "auth not resolved yet" with "signed out."
IDefaultAccountService.currentDefaultAccountis a synchronous getter that returnsnullfor everyone until the first async resolution completes — and that initialnull → accountassignment fires noonDidChangeDefaultAccountevent. So during the startup gap a signed-in user reads as signed-out. Meanwhile Claude churns native → proxy as the host forwards the token (advertisingrequired:false, thenrequired:true), repeatedly re-driving the reaction. The modal gets raised on the transientnull, and because resolution is event-silent, nothing ever tears it back down.The fix
Gate both reactive consumers on a resolved-auth signal, learned via a one-shot
getDefaultAccount()await — the only reliable indicator that resolution happened, since the initial assignment is event-silent:sessionsSetUpService.ts(the modal):_onUsableWithoutGitHubChangednow no-ops unless the state is genuinelySignedOut, so gap-time Claude churn is inert and the normal sign-in watch owns the signed-in path.agentHostDiscoveredConfigNotification.ts(the nudge):_update()returns early whileUnresolvedinstead of flashing the signed-out nudge.conditionalAuthState()helper (Unresolved | SignedIn | SignedOut) insessionsAuthGate.ts— single source of truth for the unresolved-vs-signed-out distinction.The intended signed-out conditional-auth behaviour from #328990 is fully preserved; the change only suppresses action during the unresolved window.
Validation
conditionalAuthStatetest covers all four(resolved, signedIn)combinations; existing nudge test still passes (2/2).allowSignedOutWhenUsable: true):Showing sign-in dialoglog lines: 0 — modal never raised.required:false+ 10required:trueprotected-resource transitions, plus token-forward/proxy markers) — so the reaction was exercised repeatedly and stayed inert.dialogCount:0, no "Sign in to use Agents" text, no loading overlay, signed-in button present.🤖 Generated with Claude Code