Skip to content

Commit 63d59d2

Browse files
Don't flash sign-in modal at signed-in users during Agents window startup (#329269)
* sessions: don't flash sign-in modal at signed-in users during startup 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> * sessions: replay usability state once the account resolves 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> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent b8769d2 commit 63d59d2

4 files changed

Lines changed: 122 additions & 10 deletions

File tree

src/vs/sessions/browser/sessionsAuthGate.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,38 @@ export function isAllowSignedOutWhenUsableEnabled(configurationService: IConfigu
1919
return configurationService.getValue<boolean>(AgentHostAllowSignedOutWhenUsableSettingId) === true;
2020
}
2121

22+
/**
23+
* How the conditional-auth UI should treat the current default-account snapshot.
24+
* The crucial distinction is {@link Unresolved}: on startup
25+
* {@link IDefaultAccountService.currentDefaultAccount} reads `null` for everyone
26+
* until the first async resolution completes — and that resolution fires no
27+
* change event. Treating that transient `null` as {@link SignedOut} flashes a
28+
* sign-in modal / nudge at a signed-in user during the gap, one that nothing then
29+
* retires. So consumers must ignore the account while {@link Unresolved}.
30+
*/
31+
export const enum ConditionalAuthState {
32+
/** Not resolved yet — treat as unknown; act on neither the signed-in nor signed-out branch. */
33+
Unresolved,
34+
/** Resolved: a GitHub account is signed in. */
35+
SignedIn,
36+
/** Resolved: no GitHub account is signed in. */
37+
SignedOut,
38+
}
39+
40+
/**
41+
* Collapse "has the account resolved yet?" and "is one signed in?" into the
42+
* single state the conditional-auth consumers branch on, so neither re-derives it
43+
* (and neither can independently regress the unresolved-vs-signed-out
44+
* distinction). `signedIn` must be read from the account snapshot only; this
45+
* helper decides whether that snapshot can be trusted yet.
46+
*/
47+
export function conditionalAuthState(accountResolved: boolean, signedIn: boolean): ConditionalAuthState {
48+
if (!accountResolved) {
49+
return ConditionalAuthState.Unresolved;
50+
}
51+
return signedIn ? ConditionalAuthState.SignedIn : ConditionalAuthState.SignedOut;
52+
}
53+
2254
/**
2355
* Whether a signed-out user can work without GitHub right now: the opt-in is on
2456
* and some registered session type reports that it runs without a GitHub

src/vs/sessions/browser/sessionsSetUpService.ts

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ import { IMarkdownRendererService } from '../../platform/markdown/browser/markdo
2828
import { WELCOME_COMPLETE_KEY } from '../common/welcome.js';
2929
import { SessionsWelcomeVisibleContext } from '../common/contextkeys.js';
3030
import { ISessionsManagementService } from '../services/sessions/common/sessionsManagement.js';
31-
import { observeUsableWithoutGitHub } from './sessionsAuthGate.js';
31+
import { ConditionalAuthState, conditionalAuthState, observeUsableWithoutGitHub } from './sessionsAuthGate.js';
3232

3333
import { IConfigurationService } from '../../platform/configuration/common/configuration.js';
3434
import { Codicon } from '../../base/common/codicons.js';
@@ -76,6 +76,15 @@ class SessionsSetUpWidget extends Disposable {
7676
private _initialSetupFlow = true;
7777
/** True while the window is open for a signed-out user via the conditional-auth opt-in. */
7878
private _proceedingSignedOut = false;
79+
/**
80+
* Set once the initial default-account resolution has completed. Until then
81+
* the synchronous {@link IDefaultAccountService.currentDefaultAccount} snapshot
82+
* is `null` even for a signed-in user, so a `null` reading means "not known
83+
* yet", not "signed out". The conditional-auth reaction stays inert until this
84+
* flips, otherwise it forces a sign-in modal on a signed-in user during the
85+
* startup gap — one nothing can retire, since the account resolves silently.
86+
*/
87+
private _accountResolved = false;
7988
/** Whether a signed-out user can work without GitHub right now. */
8089
private readonly _usableWithoutGitHub: IObservable<boolean>;
8190

@@ -112,13 +121,17 @@ class SessionsSetUpWidget extends Disposable {
112121
}
113122

114123
/**
115-
* The last-resort gate's answer changed while the window is open. Signed-in
116-
* users are unaffected. Becoming usable retires an already-open sign-in
117-
* modal (it was raised before the answer resolved); becoming unusable falls
118-
* back to demanding sign-in.
124+
* The last-resort gate's answer changed while the window is open. Ignored
125+
* until the account has resolved (see {@link _accountResolved}) and for
126+
* signed-in users. For a signed-out user, becoming usable retires an
127+
* already-open sign-in modal (it was raised before the answer resolved);
128+
* becoming unusable falls back to demanding sign-in.
119129
*/
120130
private _onUsableWithoutGitHubChanged(usable: boolean): void {
121-
if (this.defaultAccountService.currentDefaultAccount !== null) {
131+
// Only act once the account has resolved AND the user is signed out; while
132+
// unresolved or signed in, the sign-in watch owns the decision.
133+
const signedIn = this.defaultAccountService.currentDefaultAccount !== null;
134+
if (conditionalAuthState(this._accountResolved, signedIn) !== ConditionalAuthState.SignedOut) {
122135
return;
123136
}
124137
if (!usable) {
@@ -141,6 +154,27 @@ class SessionsSetUpWidget extends Disposable {
141154
return;
142155
}
143156

157+
// Learn when the default account resolves so the conditional-auth reaction
158+
// can tell "signed out" from "not resolved yet". On first load the account
159+
// is populated silently (no change event fires), so awaiting it once is the
160+
// only signal that resolution has happened.
161+
this.defaultAccountService.getDefaultAccount().then(() => {
162+
if (this._store.isDisposed) {
163+
return;
164+
}
165+
this._accountResolved = true;
166+
// A `_usableWithoutGitHub` change during the unresolved window was
167+
// ignored above. If the agent ended up usable, replay it now so a
168+
// signed-out user is let in rather than stranded on a sign-in dialog
169+
// nothing else retires — the web path has no post-resolution re-check
170+
// of its own (the native paths re-read usability after they await the
171+
// account). While not usable, the initial setup flow still owns the
172+
// dialog, so there is nothing to replay.
173+
if (this._usableWithoutGitHub.get()) {
174+
this._onUsableWithoutGitHubChanged(true);
175+
}
176+
});
177+
144178
if (isWeb) {
145179
void this._checkWebAuth().finally(() => this._initialSetupFlow = false);
146180
this._watchWebAuth();

src/vs/sessions/contrib/providers/agentHost/browser/agentHostDiscoveredConfigNotification.ts

Lines changed: 28 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import { SessionType } from '../../../../../workbench/contrib/chat/common/chatSe
1616
import { SessionTypeAuthRequirement } from '../../../../services/sessions/common/session.js';
1717
import { ISessionsManagementService } from '../../../../services/sessions/common/sessionsManagement.js';
1818
import { ChatInputNotificationActionKind, ChatInputNotificationSeverity, IChatInputNotificationService } from '../../../../../workbench/contrib/chat/browser/widget/input/chatInputNotificationService.js';
19-
import { isAllowSignedOutWhenUsableEnabled, shouldShowDiscoveredConfigNudge } from '../../../../browser/sessionsAuthGate.js';
19+
import { ConditionalAuthState, conditionalAuthState, isAllowSignedOutWhenUsableEnabled, shouldShowDiscoveredConfigNudge } from '../../../../browser/sessionsAuthGate.js';
2020

2121
const DISCOVERED_CONFIG_NOTIFICATION_ID = 'agentHost.discoveredConfig.claude';
2222

@@ -54,6 +54,13 @@ export class AgentHostDiscoveredConfigNotificationContribution extends Disposabl
5454
static readonly ID = 'sessions.contrib.agentHostDiscoveredConfigNotification';
5555

5656
private _shown = false;
57+
/**
58+
* Set once the initial default-account resolution has completed. Until then
59+
* {@link IDefaultAccountService.currentDefaultAccount} reads as `null` even for
60+
* a signed-in user, so the nudge stays suppressed to avoid flashing at a
61+
* signed-in user during the startup gap.
62+
*/
63+
private _accountResolved = false;
5764

5865
constructor(
5966
@IChatInputNotificationService private readonly _chatInputNotificationService: IChatInputNotificationService,
@@ -81,10 +88,28 @@ export class AgentHostDiscoveredConfigNotificationContribution extends Disposabl
8188
this._storageService.onDidChangeValue(StorageScope.APPLICATION, MUTED_STORAGE_KEY, this._store),
8289
)(() => this._update()));
8390

84-
this._update();
91+
// Until the account resolves, `currentDefaultAccount === null` reads as
92+
// "signed out" and would flash this signed-out nudge at a signed-in user
93+
// during startup. The account loads silently (no change event fires), so
94+
// await the first resolution, then re-evaluate.
95+
this._defaultAccountService.getDefaultAccount().then(() => {
96+
if (this._store.isDisposed) {
97+
return;
98+
}
99+
this._accountResolved = true;
100+
this._update();
101+
});
85102
}
86103

87104
private _update(): void {
105+
// While the account is unresolved, `currentDefaultAccount` is null for
106+
// everyone; treating that as "signed out" flashes the nudge at a signed-in
107+
// user. Nothing is shown yet, so there is nothing to tear down — just wait.
108+
const authState = conditionalAuthState(this._accountResolved, this._defaultAccountService.currentDefaultAccount !== null);
109+
if (authState === ConditionalAuthState.Unresolved) {
110+
return;
111+
}
112+
88113
// The Claude agent-host session type, once the host has advertised it.
89114
// Two providers (local / remote agent host) can offer the same id, so
90115
// prefer a usable instance and fall back to any for the display label.
@@ -94,7 +119,7 @@ export class AgentHostDiscoveredConfigNotificationContribution extends Disposabl
94119
const claude = claudeTypes.find(type => type.authRequirement === SessionTypeAuthRequirement.None) ?? claudeTypes[0];
95120

96121
const show = shouldShowDiscoveredConfigNudge({
97-
signedIn: this._defaultAccountService.currentDefaultAccount !== null,
122+
signedIn: authState === ConditionalAuthState.SignedIn,
98123
allowSignedOutWhenUsable: isAllowSignedOutWhenUsableEnabled(this._configurationService),
99124
usableWithoutGitHub: claude?.authRequirement === SessionTypeAuthRequirement.None,
100125
muted: this._storageService.getBoolean(MUTED_STORAGE_KEY, StorageScope.APPLICATION, false),

src/vs/sessions/test/browser/sessionsAuthGate.test.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,33 @@
55

66
import assert from 'assert';
77
import { ensureNoDisposablesAreLeakedInTestSuite } from '../../../base/test/common/utils.js';
8-
import { shouldShowDiscoveredConfigNudge } from '../../browser/sessionsAuthGate.js';
8+
import { ConditionalAuthState, conditionalAuthState, shouldShowDiscoveredConfigNudge } from '../../browser/sessionsAuthGate.js';
99

1010
suite('Sessions - Auth Gate', () => {
1111

1212
ensureNoDisposablesAreLeakedInTestSuite();
1313

14+
test('conditionalAuthState treats an unresolved account as unknown, never signed out', () => {
15+
// The root-cause distinction: before the account resolves, its snapshot is
16+
// null for signed-in and signed-out users alike, so `accountResolved: false`
17+
// must map to Unresolved regardless of the (untrustworthy) signedIn snapshot —
18+
// otherwise the conditional-auth UI flashes a sign-in modal at a signed-in
19+
// user during startup.
20+
const cases = [
21+
{ accountResolved: false, signedIn: false },
22+
{ accountResolved: false, signedIn: true },
23+
{ accountResolved: true, signedIn: false },
24+
{ accountResolved: true, signedIn: true },
25+
];
26+
27+
assert.deepStrictEqual(cases.map(c => conditionalAuthState(c.accountResolved, c.signedIn)), [
28+
ConditionalAuthState.Unresolved,
29+
ConditionalAuthState.Unresolved,
30+
ConditionalAuthState.SignedOut,
31+
ConditionalAuthState.SignedIn,
32+
]);
33+
});
34+
1435
test('shows the discovered-config nudge only when signed out, opted in, the type is usable without GitHub, and not muted', () => {
1536
// Independent source of truth: the nudge is the calm inverse of the gate —
1637
// it appears iff the user is signed out AND the opt-in is on AND that type

0 commit comments

Comments
 (0)